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

jenkins: osx10.15 release for all versions #2203

Merged
merged 1 commit into from
Apr 1, 2020
Merged

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 4, 2020

This lands on top of #2202, after we get nodejs/node#31459 backported to 12.x and 10.x which happens after we successfully get a 13.x release or two out.

See timeline in #2199 (comment)

@rvagg
Copy link
Member Author

rvagg commented Mar 30, 2020

Rebased, should be ready to go but would really appreciate sanity checking. What I intend:

  • osx 10.10 is only used to test Node 10, no releases
  • osx 10.11 is only used to test Node 10, 12, 13, no releases
  • osx 10.15 tests all versions and builds releases for all versions

Comment on lines 104 to 108
[ /^osx1010/, testType, gte(11) ],
[ /^osx1010/, releaseType, noVer ],
[ /^osx1011/, testType, gte(14) ],
[ /^osx1015/, releaseType, lt(13) ],
[ /^osx1011/, releaseType, noVer ],
[ /^osx1015/, releaseType, allVer ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've been reminded that these are exclusions, so noVer and allVer should be the other way around?

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the no/all ver specs are inverted from the intended.

[ /^osx1011/, releaseType, lt(11) ],
[ /^osx1011/, releaseType, gte(13) ],
[ /^osx1010/, testType, gte(11) ],
[ /^osx1010/, releaseType, noVer ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[ /^osx1010/, releaseType, noVer ],
[ /^osx1010/, releaseType, allVer ],

For "all versions" don't release on 10.10

[ /^osx1011/, testType, gte(14) ],
[ /^osx1015/, releaseType, lt(13) ],
[ /^osx1011/, releaseType, noVer ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[ /^osx1011/, releaseType, noVer ],
[ /^osx1011/, releaseType, allVer ],

for "all versions", don't release on 10.11

[ /^osx1011/, testType, gte(14) ],
[ /^osx1015/, releaseType, lt(13) ],
[ /^osx1011/, releaseType, noVer ],
[ /^osx1015/, releaseType, allVer ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[ /^osx1015/, releaseType, allVer ],
[ /^osx1015/, releaseType, noVer ],

Exclude 10.15 for release build on "no versions" -- which is a no-op, it should just be removed.

@rvagg rvagg force-pushed the rvagg/osx1015-all-release branch from 7961973 to ae48e72 Compare March 31, 2020 00:01
@rvagg
Copy link
Member Author

rvagg commented Mar 31, 2020

Thanks fellas! This script is starting to feel more like a footgun. Inverted those noVer and allVers, ptal.

@richardlau
Copy link
Member

Thanks fellas! This script is starting to feel more like a footgun.

Yeah, I think it's easier to think of inclusions (e.g. run tests on 14 and above) rather than exclusions (e.g. don't run tests on 13 and below) but the number of entries in the table makes me hesitant to flip the conditions. On the other hand making it inclusions would make retiring hosts easier as we would gradually shrink the inclusions for the host and then remove altogether. At the moment with exclusions we have to be very careful removing an entry from the file as no entry implies run everywhere.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Since noVer never does anything, its only a form of doc comment, but I think its more confusing than helpful for this purpose. An actual comment would be more clear for humans.

jenkins/scripts/VersionSelectorScript.groovy Outdated Show resolved Hide resolved
@rvagg rvagg force-pushed the rvagg/osx1015-all-release branch from ae48e72 to 6ac957b Compare April 1, 2020 02:07
@MylesBorins
Copy link
Member

Looks like this is good to go! We are planning an LTS release for 10.x and 12.x next week so would be good to get this landed ASAP so we can build an RC with the notary stuffs

Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rvagg rvagg merged commit 050fb97 into master Apr 1, 2020
@rvagg rvagg deleted the rvagg/osx1015-all-release branch April 1, 2020 21:56
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

6 participants