-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fixes & easier local dev #312
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
source "https://rubygems.org" | ||
|
||
ruby '>= 2.5', '< 2.6' | ||
ruby '>= 2.5', '< 2.7' | ||
|
||
gem "html-proofer", "~> 3.19" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
|
||
set -e | ||
|
||
node-sass \ | ||
npx --yes node-sass \ | ||
--output-style=compressed \ | ||
source/_sass/all.scss \ | ||
output_dev/css/all.css | ||
output_dev/css/all.css || true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would you make it like this? Having a failure should be reported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NPX is available within this container as it's the standard docker nodejs. I pipe to true which does not change logging, but is unfortunate because despite succeeding node-sass was emitting warnings, and therefore giving non-zero error code. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,22 +23,18 @@ services: | |
|
||
ruby: | ||
build: | ||
context: docker/ruby | ||
context: ruby:2.7-alpine | ||
user: 1000:1000 | ||
working_dir: /fig-website | ||
volumes: | ||
- .:/fig-website | ||
entrypoint: 'sh' | ||
|
||
node-sass: | ||
image: catchdigital/node-sass:8.12.0-alpine | ||
image: node:16-alpine | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here too: we need to check which version are we running inside the PSH pipeline, and align to that one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I've not checked the inner script will work with the years out of date There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No it does not. It uses the PSH inner workings, see https://docs.platform.sh/languages/php.html To be fair, it's been years since that was set up, so maybe we can review the docs and see if we can improve/refactor the deploy process. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there are failures, I'm happy to try to accommodate not forcing that upgrade path; but without an identical environment, reproducing any issues that platform may have would be next to impossible for me. Admittedly this is not the greatest impression There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the log of the current failing build. I think CSS is not getting generated:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Jean, take note that this is not even trying to run the node-sass command as far as I can see. |
||
user: 1000:1000 | ||
working_dir: /fig-website | ||
volumes: | ||
- .:/fig-website | ||
command: | ||
- 'node-sass' | ||
- '--output-style=compressed' | ||
- '--watch' | ||
- 'source/_sass/all.scss' | ||
- 'output_dev/css/all.css' | ||
- ./bin/build-sass.sh |
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.
I'm not sure this is sensible. The Ruby version that we're allowed to use is bound to the one available in the Platform.sh pipeline, which is in turn bound to whatever is available inside their PHP image.
Maybe we can increase the Ruby version by upgrading the PHP version?
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.
The non-PHP language runtimes should not be tied to PHP version. Perhaps I am misunderstanding your point though. This does not prevent running with 2.5 as the lower bound is correct. This merely enables running with a higher upper-bound (tested working). The docker for example still uses ruby 2.5 (also the proof-reading it checks for reports numerous failures, but perhaps that can be solved separately).
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.
The Ruby runtime is unfortunately tied due to how the deploy pipeline works: with a single container. So, inside that single container, we require a specific PHP version, and inside we find another, specific, not-chosen-by-us Ruby version. That's why I was saying that upgrading PHP may lead to upgrading Ruby.
Hence, running with a different Ruby version locally is not good IMHO, because it 's different from what will happen during the deploy.
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.
So the ruby you are bound to use won't change from this line is I guess the confusion I am having. This allows the same lower and upper bound range of ruby runtimes, but additionally, adds the flexibility to not rely on (deprecated, end-of-life) ruby 2.6 [source[(https://endoflife.date/ruby)
I understand a little about how a box might have only a single runtime. So running this on that box should not break. If it does, please can you share the message?
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.
My concern is that you may start having issues in the pipeline in PSH while deploying, while being unaware of that during local development, since you would be using a different version.
I know that it's EOL, and I'm tolerating it just because we're not actively deploying it, we're just using it to compile CSS and check links... But, in the end, I would be pretty happy to upgrade to PHP 8.1 and obtain a bump on Ruby too.
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.
Any progress on sharing failure messages? I'm more or less stuck on fixing without.