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

Use shell expansion when selecting which NODE_VERSION to build #322

Merged
merged 5 commits into from
Jul 25, 2017
Merged

Use shell expansion when selecting which NODE_VERSION to build #322

merged 5 commits into from
Jul 25, 2017

Conversation

chorrell
Copy link
Contributor

@chorrell chorrell commented Feb 1, 2017

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 :)

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.)
@LaurentGoderre
Copy link
Member

Really? That would be great if it works!

@chorrell
Copy link
Contributor Author

chorrell commented Feb 1, 2017

Yeah, it seems to work pretty well. I just pushed a small change to test-build.sh to call out which major.minor version is being tested.

@chorrell
Copy link
Contributor Author

chorrell commented Feb 1, 2017

Also, to verify I did this simple test from within the repo:

$ versions=4*
$ versions=( "${versions[@]%/}" )
$ echo $versions
4.7

@chorrell
Copy link
Contributor Author

chorrell commented Feb 1, 2017

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 :)

@LaurentGoderre
Copy link
Member

@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

@chorrell
Copy link
Contributor Author

chorrell commented Feb 1, 2017

@LaurentGoderre I was pondering that scenario too and I think it's very unlikely we would do that.

@tianon
Copy link
Contributor

tianon commented Feb 1, 2017 via email

@chorrell
Copy link
Contributor Author

chorrell commented Feb 1, 2017

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 Directory: would change. And we'd need to update the logic in generate-stackbrew-library.sh that generates the values for Tags:

@LaurentGoderre
Copy link
Member

@tianon How would the Travis matrix be updated though?

@tianon
Copy link
Contributor

tianon commented Feb 1, 2017

@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

@LaurentGoderre
Copy link
Member

I think I would prefer to keep it simple and only expand the ones that need it

env:
    matrix :
        NODE_VERSION: '4.*'
        NODE_VERSION: '6.1'
        NODE_VERSION: '6.2'
        NODE_VERSION: '7.*'

@chorrell
Copy link
Contributor Author

Is this something we can merge for now and then consider folder renaming in a separate issue/PR?

@SimenB SimenB mentioned this pull request Apr 12, 2017
nschonni

This comment was marked as off-topic.

@nschonni
Copy link
Member

@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

@chorrell
Copy link
Contributor Author

The email address was removed from my profile because I don't work there anymore.

SimenB

This comment was marked as off-topic.

@SimenB
Copy link
Member

SimenB commented Jul 25, 2017

@chorrell can you resolve the conflict?

@SimenB
Copy link
Member

SimenB commented Jul 25, 2017

Hah, I could do it in the web gui 😄

@chorrell
Copy link
Contributor Author

Nice! Are we good to merge this now?

cc @nodejs/docker

@chorrell
Copy link
Contributor Author

Huh, why did this fail?

https://travis-ci.org/nodejs/docker-node/jobs/257409744

@SimenB
Copy link
Member

SimenB commented Jul 25, 2017

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)

@SimenB
Copy link
Member

SimenB commented Jul 25, 2017

I broke it when merging #328

@chorrell
Copy link
Contributor Author

Ah, ok.

@SimenB SimenB merged commit 7fd6af2 into nodejs:master Jul 25, 2017
@chorrell chorrell deleted the travis-shell-expansion branch July 25, 2017 19:02
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.

None yet

5 participants