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

Update Sass GitHub action to save built files #4929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update Sass GitHub action to save built files #4929

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions .github/workflows/sass.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,14 @@ jobs:
run: |
! grep "\$govuk-" .tmp/all.css

- name: Save compiled Sass
uses: actions/[email protected]
if: ${{ !cancelled() }}
Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Member Author

@romaricpascal romaricpascal Apr 15, 2024 •
edited
Loading

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.

with:
name: Dart Sass v1.0.0 output
Copy link
Member Author

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.

Copy link
Contributor

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? 😕

Copy link
Member Author

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.

path: .tmp/all.css
if-no-files-found: ignore

dart-sass-latest:
name: Dart Sass v1 (latest)
runs-on: ubuntu-latest
Expand Down Expand Up @@ -80,6 +88,14 @@ jobs:
run: |
! grep "\$govuk-" .tmp/all.css

- name: Save compiled Sass
uses: actions/[email protected]
if: ${{ !cancelled() }}
with:
name: Dart Sass v1 (latest) output
path: .tmp/all.css
if-no-files-found: ignore

# Node Sass v3.4.0 = LibSass v3.3.0
lib-sass:
name: LibSass v3.3.0 (deprecated)
Expand Down Expand Up @@ -110,6 +126,14 @@ jobs:
run: |
! grep "\$govuk-" .tmp/all.css

- name: Save compiled Sass
uses: actions/[email protected]
if: ${{ !cancelled() }}
with:
name: LibSass v3.3.0 (deprecated) output
path: .tmp/all.css
if-no-files-found: ignore

# Node Sass v8.x = LibSass v3 latest
lib-sass-latest:
name: LibSass v3 (latest, deprecated)
Expand Down Expand Up @@ -140,6 +164,14 @@ jobs:
run: |
! grep "\$govuk-" .tmp/all.css

- name: Save compiled Sass
uses: actions/[email protected]
if: ${{ !cancelled() }}
with:
name: LibSass v3 (latest, deprecated) output
path: .tmp/all.css
if-no-files-found: ignore

ruby-sass:
name: Ruby Sass v3.4.0 (deprecated)
runs-on: ubuntu-latest
Expand Down Expand Up @@ -168,6 +200,14 @@ jobs:
run: |
! grep "\$govuk-" .tmp/all.css

- name: Save compiled Sass
uses: actions/[email protected]
if: ${{ !cancelled() }}
with:
name: Ruby Sass v3.4.0 (deprecated) output
path: .tmp/all.css
if-no-files-found: ignore

ruby-sass-latest:
name: Ruby Sass v3 (latest, deprecated)
runs-on: ubuntu-latest
Expand Down Expand Up @@ -195,3 +235,11 @@ jobs:
- name: Check output
run: |
! grep "\$govuk-" .tmp/all.css

- name: Save compiled Sass
uses: actions/[email protected]
if: ${{ !cancelled() }}
with:
name: Ruby Sass v3 (latest, deprecated) output
path: .tmp/all.css
if-no-files-found: ignore