-
Notifications
You must be signed in to change notification settings - Fork 119
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
feat: add 3.0.1 #160
feat: add 3.0.1 #160
Conversation
donstephan
commented
Jul 18, 2024
- support for 3.0.1 running with node 20.15.1
- removed the need for npm-check-updates installed globalled in favor of running with npx
- removed the need for npm-check-updates installed globalled in favor of running with npx
@GeoffreyBooth could you please merge? |
What about version 3.0.0? |
update.sh
Outdated
@@ -126,7 +119,7 @@ cd example/app | |||
|
|||
meteor update --release "${new_meteor_version}" | |||
|
|||
npm-check-updates --configFilePath /dev/null --upgrade | |||
npx npm-check-updates --configFilePath /dev/null --upgrade |
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 the use of npx
be split out into its own PR? The version bump PRs should be essentially fully automatic.
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.
Yes I'll split this up.
# if we're using node version 14.21.4 | ||
# we need to get the meteor/node image | ||
# else we use the official node image |
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.
# if we're using node version 14.21.4 | |
# we need to get the meteor/node image | |
# else we use the official node image | |
# For Node 14.21.4 we need to get the `meteor/node` image; otherwise we can use the official Node image |
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.
Images were using the official Node image for 14.21.4 before this PR; why does this need to change? This also feels like it should be a separate PR, as it’s unrelated to adding Meteor 3.0.1.
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.
Tests were failing due to it trying to reference node:14.21,4-alpine3.17
which only exists in the meteor/node
registry. I'm really not sure how this logic was working before I made this change.
This sed statement would have no effect on this line:
export node_version=14.21.3
echo "FROM meteor/node:14.21.4-alpine3.17" | sed "s|FROM node:.*|FROM node:${node_version}-alpine|"
So I suspect all tests were running using meteor/14.21.4-alpine3.17
but there's no output in old tests to confirm this. The test output changed in #144 is only informative and not alter that sed logic.
Here's output from the PR before I made this change: https://github.com/disney/meteor-base/actions/runs/9994011488/job/27622819639#step:4:30
Do you want this in a separate PR? This is a blocker for tests to pass.
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 I suspect all tests were running using
meteor/14.21.4-alpine3.17
but there’s no output in old tests to confirm this.
There’s GitHub Actions output for every PR that’s been merged in. Here’s the last one: https://github.com/disney/meteor-base/actions/runs/9227042103/job/25388152875
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 going to close this and split it into separate PRs so this is more clear.
There doesn't seem to be a |