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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use dartsass for publisher css compilation #770

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

mtaylorgds
Copy link
Contributor

What

Change docker compose for Mainstream Publisher to match compilation of CSS using dart-sass. See alphagov/publisher#2278.

Why

We're switching these apps from libsass to dart-sass, and this change is needed to support that.

@@ -44,7 +44,7 @@ services:
BINDING: 0.0.0.0
expose:
- "3000"
command: bin/rails s --restart
command: bin/dev
Copy link
Member

@kevindew kevindew Aug 9, 2024

Choose a reason for hiding this comment

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

Something that is a bit weird about this approach (and appreciate horse has bolted for a bunch of our repos) is that bin/dev is going to be managing multiple processes which isn't typical behaviour for docker. It raises quite a wtf for apps like this where there is also a worker process - why have that managed differently in docker to css?

We should also manage all dev processes from the Procfile so the worker process should also be defined there, not just web and css.

I'd suggest you take the approach we've done in govuk-chat and have each individual process be it's own docker process:

command: bin/dev web
govuk-chat-css:
<<: *govuk-chat
command: bin/dev css

Copy link
Contributor

Choose a reason for hiding this comment

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

I have tried this approach locally and found that it does not run Sass in watch mode (as described here). I don't know if that is inherent in the approach or if it requires a change in this file to run.

Copy link
Member

Choose a reason for hiding this comment

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

it will be running the exact same process

Copy link
Contributor

@davidtrussler davidtrussler Aug 12, 2024

Choose a reason for hiding this comment

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

What I am seeing in the console as I start publisher is shown below, comparing the original changes with what is suggested here, which suggests the command Execute dartsass:watch is not being called. I guess that will require some changes in the config in Publisher as well.

Screenshot 2024-08-12 at 12 36 00 Screenshot 2024-08-12 at 12 30 15
Screenshot 2024-08-12 at 12 21 34 Screenshot 2024-08-12 at 12 21 50

Copy link
Member

Choose a reason for hiding this comment

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

For the latter you need to execute publisher-css as a separate process and/or flag it as a dependency of the process - as per how the worker is configured,

The former example you've got there is unconventional for docker as it's managing 2 processes

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that's making sense (and also working as expected 😄) so I have updated accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Great 😄 you'll just need to edit:

depends_on:
- redis
- mongo-3.6
- nginx-proxy
- publishing-api-app
- link-checker-api-app
- publisher-worker

to include publisher-css, which will mean that css is automatically started when web is started.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, have done that as well. 👍

@davidtrussler davidtrussler force-pushed the migrate-publisher-to-dart-sass branch 2 times, most recently from bda351e to 9fbfaea Compare August 12, 2024 11:06
@davidtrussler davidtrussler merged commit fd414e8 into main Aug 28, 2024
2 checks passed
@davidtrussler davidtrussler deleted the migrate-publisher-to-dart-sass branch August 28, 2024 13:31
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.

3 participants