Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fix Docker and upgrade Node #415
Fix Docker and upgrade Node #415
Changes from all commits
3c64705
7e66225
3bf256a
a2b6ca2
5afbab2
b28e273
53a4f3e
ebd1be5
dc866d2
596ebdd
1aa51c0
0e6ee65
2b1c742
ab76abb
0924268
08f986c
0634f9d
d8498ea
7fcee93
565a4c3
d40a7cc
9a400b8
4aea59e
ef25374
8ce35b8
8687fd7
d93e3a5
d74bd4d
27e15ca
f83e2f9
a2dd673
211eafc
d2841b3
daa6589
bf85545
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Added the --immutable flag here as a precaution. The Docker build will fail if the Yarn install process would result in a different yarn.lock file, which could happen for example if two different versions of Yarn are used.
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.
This command doesn't actually exist anymore but it was never updated. Probably because the image is never run with this default command but rather a command override is supplied by Docker Compose.
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.
Is there a reason why splitting commands then explanations?
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 believe you can copy and paste the commands all at once
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.
Can't actually pass these options this way.
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.
This was the real source of the "node_modules state file not found" error, not the Yarn 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.
These are really the only folders that can be watched for hot-reloading. Mounting everything was causing the .env file (which gets copied in the build step, see the Dockerfile) to go missing.
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. My thinking here is that we pin both of the dev environments (Docker and Conda) to the same specific version (22.8.0), but we relax the constraint here. And to make things easier on ourselves, we drop official support for Node 18.
My understanding is that changing the value in package.json will not prevent somebody from running the app with Node 18; it will just warn them. However:
yarn install
after making this change, I did not get any warningsnpm install
(v8.19.2), I did get a warning (but just a warning, it didn't exit with an error):npm WARN EBADENGINE Unsupported engine
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 thinking behind pinning the dev versions to the same version is just to avoid the possibility of a node version related bug appearing in one environment but not the other and causing confusion
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 only hesitation about pinning on the conda environment, as listed before, is that with hard pins, one has to be quite disciplined about updating them regularly. Otherwise, one misses out on security updates and the like.
I know this is true of the Docker image too, but unless it is infra/core system deps or to avoid hard incompatibilities then it is best to not add a hard pin to
major.minor
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.
Ah yes, what you write about security updates makes total sense to me. I'll relax the Node.js requirement in the Conda and Docker environments.