-
Notifications
You must be signed in to change notification settings - Fork 2k
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 shell expansion when selecting which NODE_VERSION to build #322
Conversation
With this change we no longer need to update the .travis.yml file for each minor update of a given Node.js image. Bash shell expansion will determine the right version to build based on the available directories (4.7, 6.9, 7.5 etc.)
Really? That would be great if it works! |
Yeah, it seems to work pretty well. I just pushed a small change to |
Also, to verify I did this simple test from within the repo:
|
Testing looks good and it does seem to be doing the right thing. I'd like to leave this open for a bit for review though :) |
@chorrell is there any reason where we would have to support two separate minor versions for a major version? If we did, it still wouldn't block this, we would just have to expand just that version |
@LaurentGoderre I was pondering that scenario too and I think it's very unlikely we would do that. |
Another solution would be to simply rename the folders to be the minimum
supported specificity (see https://github.com/docker-library/elasticsearch
for an example -- it has 1.7, 2.4, and 5 as folders).
|
I'd be fine with that too :) If we go the dir renaming route, I guess we'd need to generate a new library file since the values of |
@tianon How would the Travis matrix be updated though? |
@LaurentGoderre the Travis matrix would reference the folders -- https://github.com/docker-library/elasticsearch/blob/ffaaf3283e47dcb732e90288a58757b87a8439a7/.travis.yml#L4-L10 env:
- VERSION=5 VARIANT=
- VERSION=5 VARIANT=alpine
- VERSION=2.4 VARIANT=
- VERSION=2.4 VARIANT=alpine
- VERSION=1.7 VARIANT=
- VERSION=1.7 VARIANT=alpine |
I think I would prefer to keep it simple and only expand the ones that need it
|
Is this something we can merge for now and then consider folder renaming in a separate issue/PR? |
@chorrell looks like the first commits were done with an email not associated with your GH account, might want to add it in your profile |
The email address was removed from my profile because I don't work there anymore. |
@chorrell can you resolve the conflict? |
Hah, I could do it in the web gui 😄 |
Nice! Are we good to merge this now? cc @nodejs/docker |
Huh, why did this fail? |
I was gonna merge when I got a green CI. I'm not because of the toc being different... (Could you fix that? I'm on mobile currently.) But if 4, 6 and 8 builds successfully, I'll merge (if no one beats me to it) |
I broke it when merging #328 |
Ah, ok. |
With this change we no longer need to update the .travis.yml file for
each minor update of a given Node.js image. Bash shell expansion will
determine the right version to build based on the available directories
(4.7, 6.9, 7.5 etc.)
That being said, a sanity check on this would be good :)