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.

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

Already on GitHub? Sign in to your account

Ruby 3.1 upgrade #2296

Merged
merged 38 commits into from
Apr 26, 2023
Merged

Ruby 3.1 upgrade #2296

merged 38 commits into from
Apr 26, 2023

Conversation

drewbo
Copy link
Contributor

@drewbo drewbo commented Apr 10, 2023

Changes proposed in this pull request:

Remaining items

  • A few remaining instances of asset_url/asset_path need to be removed. As part of this, the USWDS overrides should be removed and the asset paths "hardened" (right now I swapped many of the initial paths to '../' so they find their way out of the css folder)
  • Broken docs navigation
  • Inspect all pages for broken links/images/css
  • The local dev server is deeply broken and I highly recommend not trying it (it runs too many processes in parallel)
  • image minification
  • Assess whether asset fingerprinting/integrity checks need to be added back in
  • Assess whether css can be further reduced in size/compressed
  • admin/netlify cms is broken

馃槑PREVIEW URL

Security Considerations

New dependencies, upgraded Ruby

@drewbo
Copy link
Contributor Author

drewbo commented Apr 12, 2023

Assuming I can check those last few boxes above and fix the build in the next day or so, I think the next move is to evaluate whether (1) this seems like a plausible future for cg-site and (2) seems like a plausible future for sites based on https://github.com/cloud-gov/pages-uswds-jekyll. My thoughts on how this works so far:

  • it's nice to have a single _assets folder and each of the parcel/npm scripts are fairly straightforward/readable
    • copies over documents and static js
    • uses built-in sass/postcss compilation on files in css + USWDS as specified in .sassrc
    • uses built-in image minification on images
  • it's a bit brittle but okay to use jekyll alongside another asset handler:
    • not using the assets folder breaks the ability to use {% link ... %} but having assets plus another _assets folder for files prior to preprocessing is pretty confusing (parallel pipelines)
    • the dev server situation is difficult because jekyll wants to overwrite assets in the built folder but we're putting other things there. In the build, we can run things serially, but using watchers creates a waterfall of dependencies/race conditions.
  • this isn't the core use case for parcel (it desperately tries to interact with JS files) but it seems okay at least for the build step
  • I tried my best to minimize dependencies but this does add a few as well as a number of npm scripts.

@drewbo drewbo changed the title [WIP] Ruby 3.1 upgrade Ruby 3.1 upgrade Apr 13, 2023
@drewbo drewbo requested a review from a team April 13, 2023 21:01
@drewbo
Copy link
Contributor Author

drewbo commented Apr 13, 2023

This is probably ready to merge. Remaining weak spots of things to review:

  • Deleting the current package-lock.json and reinstalling will create a different lockfile, something to do with "@parcel/transformer-sass". I'm worried that this creates some instability for new installs
  • The local dev server works but seems a little breakable and still starts too many parallel watch processes
  • Final asset/image review
  • We need to either skip SRI on DAP or get them to add CORS headers (otherwise it doesn't load)

hursey013
hursey013 previously approved these changes Apr 14, 2023
Copy link
Contributor

@hursey013 hursey013 left a comment

Choose a reason for hiding this comment

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

Few small suggestions, all just style/syntax, otherwise looks good!

@@ -13,12 +13,12 @@ jobs:

- uses: ruby/setup-ruby@v1
with:
ruby-version: 2.7
ruby-version: 3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed when using .ruby-version

bundler-cache: true # runs 'bundle install' and caches installed gems automatically

- uses: actions/setup-node@v3
with:
node-version: 14.19.1
node-version: 18.15.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Could potentially replace with node-version-file: '.nvmrc' to try to limit the number of places where we need to change the node version.

@@ -21,7 +19,6 @@ group :jekyll_plugins do
gem 'jekyll-paginate-v2', "3.0.0"
gem 'jekyll-sitemap'
gem 'jekyll-seo-tag'
gem 'jekyll-assets', git: "https://github.com/envygeeks/jekyll-assets"
Copy link
Contributor

Choose a reason for hiding this comment

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

This always made me nervous, glad to see it go.

@@ -53,16 +53,16 @@ Logs are currently retained for 180 days, and you will only see data for applica

After logging in, you'll see the App Overview dashboard.

{% asset app-overview.png alt="App Overview dashboard" %}
!["App Overview dashboard"]({{site.baseurl}}/assets/images/content/app-overview.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

Idea: alternative syntax that I gravitated towards as being more readable, but totally my own personal preference:
!["App Overview dashboard"]({{ "/assets/images/content/app-overview.png" | relative_url }})

package.json Outdated
"watch:server": "mkdir -p _site && sleep 5 && browser-sync _site -w --reload-delay 1000",
"build": "run-s prod:parcel jekyll",
"jekyll": "bundle exec jekyll build JEKYLL_ENV=production",
"prod:parcel": "run-p build:css build:static:js build:static:docs build:images",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"prod:parcel": "run-p build:css build:static:js build:static:docs build:images",
"prod:parcel": "run-p build:*",

@drewbo
Copy link
Contributor Author

drewbo commented Apr 19, 2023

Documenting for postering but I've removed the subresource integrity build process:

  • For our one third-party script, Digital Analytics Program, we received the following guidance:

It's not a good choice for calling third party scripts from a location where that org changes the code. So, from our dap.digitalgov.gov host, the hash would break whenever we pushed an updated code version. You could enable SRI if you self-host, but then you don't get our automatic feature/security/bug fixes when we push new versions. So enabling SRI is foregoing one measure of security (our maintenance of the code) for another (assuring the code has not been tampered with)

  • For local files, there isn't a clear threat model because if an attacker has access to modify our hosted JS, they either have access to the build process or the CDN, either of which mean they could circumvent the SRI check to begin with. In the abstract, I would leave the check in to as an additional layer of security but the library we're using to replace the previous jekyll assets sub-dependency doesn't easily allow for partial SRI application (adding the check to certain files and not others).

@drewbo drewbo requested a review from a team April 20, 2023 18:18
@markdboyd
Copy link
Contributor

@drewbo I did some initial checking over the preview build and here's what I found:

Also, when testing the ability to build the site locally, I found:

  • I got local changes to Gemfile.lock
  • Styles aren't loading on the local site. Relevant console error: Refused to apply style from 'http://localhost:3000/assets/css/styles.css' because its MIME type ('text/html') is not a supported stylesheet MIME type, and strict MIME checking is enabled.

Copy link
Contributor

@markdboyd markdboyd left a comment

Choose a reason for hiding this comment

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

Everything looked good when testing a local build and in the Pages build. Great work!

@drewbo drewbo merged commit 35096d5 into main Apr 26, 2023
1 check failed
@drewbo drewbo deleted the chore-ruby-3.1-upgrade branch April 26, 2023 15:26
@drewbo drewbo mentioned this pull request Apr 26, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants