-
Notifications
You must be signed in to change notification settings - Fork 328
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Sass GitHub action to save built files #4929
base: main
Are you sure you want to change the base?
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 324724b |
uses: actions/upload-artifact@v4.3.1 | ||
if: ${{ !cancelled() }} | ||
with: | ||
name: Dart Sass v1.0.0 output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick Would have wanted to re-use the name of the job by looking in one of the contexts provided to the step... but couldn't find anything. If anyone knows how, would be keen to update.
Don't think that's a blocker, though, more of a nice-to-have-if-we-know-how.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uploading the artefact is also very repetitive – we briefly explored running these using a matrix in #3030 but at the time it felt like it made the code too complicated. Wondering if that's worth revisiting.
Equally with us dropping support for Ruby Sass and LibSass 'soon' maybe it's not worth spending any more time on this? 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thought there was a fair amount of repetitions and remembered Colin's attempt at having a matrix. Still not convinced we need to DRY things up with a matrix there given it's not that costly to add something 6 times and we don't touch those that often. Especially with the change of Sass compiler support at some point in the future.
If for whatever reason we start bulking these workflows with more shared steps, we could revisit then.
324724b
to
5e1a809
Compare
5e1a809
to
324724b
Compare
@@ -45,6 +45,14 @@ jobs: | |||
run: | | |||
! grep "\$govuk-" .tmp/all.css | |||
|
|||
- name: Save compiled Sass | |||
uses: actions/upload-artifact@v4.3.1 | |||
if: ${{ !cancelled() }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick Thinking we could add a continue-on-error
to avoid that step failing the job if the upload has an issue. This continue-on-error
would have to be conditionned to the previous step being a success, though. Otherwise, I think it'd make test failures not fail the job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to guard against the job being cancelled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following GitHub's recommendations to prefer !cancelled()
to always()
to ensure a step runs regardless of the previous steps. Made sense to me as if the job was cancelled, we don't care about uploading artifacts anyways. Happy to drop a comment on the first one to remind of GitHub's recommendations.
This will allow inspecting the output should an older version of Sass fail.
Each job gets the output of the Sass compilation uploaded as artifact, which we can then download for investigation from the Summary page of our GitHub Sass workflow (accessible by clicking 'Details' next to any of the Sass checks and then 'Summary').
The artifact upload is set up as a final step for each job, so that checking there's no more Sass variables in the output runs first. It is set up with
if: ${{ !cancelled() }}
so that it runs even if an error happens beforehand (but respects the job having been cancelled, unlikealways()
).