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

Add skip/security option to update.sh #784

Merged
merged 1 commit into from
Jun 18, 2018
Merged

Add skip/security option to update.sh #784

merged 1 commit into from
Jun 18, 2018

Conversation

chorrell
Copy link
Contributor

@chorrell chorrell commented Jun 13, 2018

This adds a new -s option argument (and usage) to update.sh that allows you to skip updating the Yarn and Alpine versions when updating the images for a security release.

I also updated the shebang to match the other scripts and to ensure you get a newer (i.e. homebrew installed) version of bash when on a mac.

@nodejs-github-bot
Copy link
Collaborator

@chorrell sadly an error occured when I tried to trigger a build :(

@chorrell chorrell requested a review from a team June 13, 2018 02:14
@chorrell
Copy link
Contributor Author

What's wrong with nodejs-github-bot ?

PeterDaveHello

This comment was marked as off-topic.

SimenB

This comment was marked as off-topic.

@SimenB
Copy link
Member

SimenB commented Jun 13, 2018

Maybe worth updating the contributing guide?

PeterDaveHello

This comment was marked as off-topic.

@LaurentGoderre
Copy link
Member

@chorrell I don't think this will work for alpine like this

LaurentGoderre

This comment was marked as off-topic.

@chorrell
Copy link
Contributor Author

I made some changes to the update_node_version function update a temp Dockerfile first, then replace it with the original when done. This allowed me to get the currently used version of Alpine from the old Dockerfile which is needed when skipping the Alpine update. I think this might be generally useful since we can add logic later to show a diff of the old and new Dockerfiles when updating

@chorrell
Copy link
Contributor Author

I'm going to do some more local testing of this over the week to ensure it can handle the various update scenarios

@LaurentGoderre
Copy link
Member

Good call! I didn't think about the fact that the file gets overwritten right away!

@chorrell
Copy link
Contributor Author

I think there's still some more work to do. Doing something like this resets the yarn version to 0.0.0:

 ./update.sh -s 6 alpine

@SimenB
Copy link
Member

SimenB commented Jun 13, 2018

Rewrite in node and use https://github.com/rcjsuen/dockerfile-ast? :D

@LaurentGoderre
Copy link
Member

Maybe for the sake of simplicity, the first step of this function would be to extract the current Node version, Yarn version and Alpine version. Then depending on the setting, update it. It might be a bit of extra processing if want to do a full update but I think the script would be more readable and simpler to understand.

What do you think?

@SimenB
Copy link
Member

SimenB commented Jun 13, 2018

Extra processing is no issue - this is super quick anyways. The only thing taking time is the network calls figuring out latest version

@chorrell
Copy link
Contributor Author

I pushed another change to better deal with the Yarn version

@LaurentGoderre
Copy link
Member

LaurentGoderre commented Jun 13, 2018

@chorrell not required to land this but it might be nice to check on the flag early and if the slip flag is not defined, fetch the latest version of Yarn, then in the loop call the latest version if not skipping.

This would avoid repeated HTTP requests

LaurentGoderre

This comment was marked as off-topic.

PeterDaveHello

This comment was marked as off-topic.

PeterDaveHello

This comment was marked as off-topic.

LaurentGoderre

This comment was marked as off-topic.

LaurentGoderre

This comment was marked as off-topic.

@chorrell
Copy link
Contributor Author

I'll do a rebase to cleanup the commit history.

@chorrell
Copy link
Contributor Author

rebase done

nschonni

This comment was marked as off-topic.

@chorrell
Copy link
Contributor Author

I included some improvements to the usage instructions based on everyone's feedback

@chorrell
Copy link
Contributor Author

Oh, I noticed the Travis-CI integration has changed. Is it not using nodejs-github-bot anymore?

@chorrell
Copy link
Contributor Author

chorrell commented Jun 18, 2018

Er, k, it's using both. What's going on? I prefer having this though :)

https://github.com/nodejs/docker-node/runs/4204119

@LaurentGoderre
Copy link
Member

@chorrell #790 needs to be merged to fix you build. I think the shfmt image was updated.

@LaurentGoderre
Copy link
Member

Can you rebase?

@chorrell
Copy link
Contributor Author

done

@chorrell
Copy link
Contributor Author

Does anyone understand why we have 3 builds? I kind of get the separated branch build, but why are there 2 PR builds? Is nodejs-github-bot redundant? Did a setting change somewhere?

@chorrell
Copy link
Contributor Author

I don't recall adding this...

screen shot 2018-06-18 at 1 17 06 pm

@chorrell
Copy link
Contributor Author

I disabled since we have nodejs-github-bot will remove it entirely later once I see it's not breaking anything

@LaurentGoderre
Copy link
Member

Build passes https://travis-ci.org/nodejs/docker-node/builds/393736009 Can I merge?

@chorrell
Copy link
Contributor Author

sure

@LaurentGoderre LaurentGoderre merged commit 0af854c into master Jun 18, 2018
@LaurentGoderre LaurentGoderre deleted the skip branch June 18, 2018 19:18
@nschonni
Copy link
Member

The disabled one might be related to nodejs/build#1353

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.

6 participants