Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deploy new debug client to a CDN #5578

Merged
merged 7 commits into from
Jan 10, 2024
Merged

Deploy new debug client to a CDN #5578

merged 7 commits into from
Jan 10, 2024

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Dec 16, 2023 •
edited
Loading

Summary

This adds a new CI workflow that:

  • builds the new debug client
  • pushes the compiled output to a separate repo like this
  • adjusts the index.html so that the assets are fetched from CDN URLs
  • commits the updated index.html to the main OTP repo

This has the advantage of not needing to faff with npm when you do local development of OTP.

Issue

#5330

Documentation

Line comments.

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner December 16, 2023 16:37
@codecov Codecov
Copy link

codecov bot commented Dec 16, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (df9e1e1) 67.35% compared to head (4b3b953) 67.35%.

Additional details and impacted files 
@@              Coverage Diff              @@
##             dev-2.x    #5578      +/-   ##
=============================================
- Coverage      67.35%   67.35%   -0.01%     
  Complexity     16150    16150              
=============================================
  Files           1857     1857              
  Lines          71075    71075              
  Branches        7402     7402              
=============================================
- Hits           47872    47870       -2     
- Misses         20745    20746       +1     
- Partials        2458     2459       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leonardehrenfried
Copy link
Member Author

@testower Is it possible to override the Vite base config setting? That way we would save the ugly sed processing. (On the other hand, it's just one line.)

@testower
Copy link
Contributor

@testower Is it possible to override the Vite base config setting? That way we would save the ugly sed processing. (On the other hand, it's just one line.)

Yes have a look at the vite build cli options https://main.vitejs.dev/guide/cli.html#options-1

although I have never tested with a full url as opposed to just a path.

@testower
Copy link
Contributor

testower commented Dec 16, 2023 •
edited
Loading

I just tested this and it works:

✗ npm run build -- --base https://cdn.jsdelivr.net/gh/opentripplanner/debug-client-assets@main/2023-12-16T14-49/

outputs in index.html:

<!doctype html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <link rel="icon" type="image/svg+xml" href="https://cdn.jsdelivr.net/gh/opentripplanner/debug-client-assets@main/2023-12-16T14-49/img/otp-logo.svg" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>OTP Debug Client</title>
    <script type="module" crossorigin src="https://cdn.jsdelivr.net/gh/opentripplanner/debug-client-assets@main/2023-12-16T14-49/assets/index-6aa94757.js"></script>
    <link rel="stylesheet" href="https://cdn.jsdelivr.net/gh/opentripplanner/debug-client-assets@main/2023-12-16T14-49/assets/index-ab84da73.css">
  </head>
  <body>
    <div id="root"></div>
    
  </body>
</html>

@testower
Copy link
Contributor

The benefit of use the base config option is that it will also fix any urls inside the code that refer to static assets. As I mentioned we'll need to fix the image imports so they are included as well.

@leonardehrenfried leonardehrenfried force-pushed the debug-client-cdn branch 10 times, most recently from 6358504 to 9cd3460 Compare December 16, 2023 20:20
@leonardehrenfried leonardehrenfried changed the title Add CI steps to deploy new debug client to a CDN Deploy new debug client to a CDN Dec 16, 2023
@testower testower mentioned this pull request Dec 18, 2023
Merged
@leonardehrenfried
Copy link
Member Author

@testower Do you have an opinion if the versioned folders should all be at the root level or put into subdirectories like 2023/12/2023-12-16T14-49. I keep going back and forth between the two options.

@testower
Copy link
Contributor

No strong opinion. I guess sub-folders will ease cleaning up.

testower
testower reviewed Dec 18, 2023
View reviewed changes
Copy link
Contributor

@testower testower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved assuming it will be rebased on top of #5579

testower
testower previously approved these changes Dec 18, 2023
View reviewed changes
@leonardehrenfried
Copy link
Member Author

I went with the year/month prefix.

@leonardehrenfried leonardehrenfried merged commit 7f92d6b into dev-2.x Jan 10, 2024
6 checks passed
@leonardehrenfried leonardehrenfried deleted the debug-client-cdn branch January 10, 2024 14:50
@leonardehrenfried
Copy link
Member Author

I'm quite surprised that this worked on the first try.

Here is what the commits to dev-2.x look like: d7e9ecd

And here are the commits to the assets repo: opentripplanner/debug-client-assets@066674f

I would have expected more assets for the modes and not just trolleybus and monorail. @testower, does this look ok to you?

@leonardehrenfried
Copy link
Member Author

Screenshot from 2024-01-10 16-11-06

It seems to work anyway and the icons are encoded as img urls. Vite seems to be doing some sort of inlining.

@testower
Copy link
Contributor

Yes this is indeed expected: https://vitejs.dev/config/build-options.html#build-assetsinlinelimit

@t2gran t2gran added this to the 2.5 (next release) milestone Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@testower testower testower approved these changes

@jtorin jtorin jtorin approved these changes

Assignees
No one assigned
Labels
improvement Skip Changelog
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.
4 participants
@leonardehrenfried @testower @jtorin @t2gran