semantic-release / travis-deploy-once

🚫Test multiple node versions on Travis. Deploy once. If all of them pass.
MIT License
33 stars 7 forks source link

Electing build leader for node stable, 8 not working #49

Closed joscha closed 6 years ago

joscha commented 6 years ago

See: https://travis-ci.org/joscha/gulp-rewrite-css/jobs/331351873

$ travis-deploy-once 'yarn semantic-release'
[Travis Deploy Once]: Electing build leader among builds with Node versions: stable, 8.
[Travis Deploy Once]: Electing job (2) as build leader as it runs the highest node version (8).
[Travis Deploy Once]: The current job (371.1) is not the build leader.
Done in 1.50s.

but:

3.50s$ nvm install stable
Downloading and installing node v9.4.0...
Downloading https://nodejs.org/dist/v9.4.0/node-v9.4.0-linux-x64.tar.xz...
Computing checksum with sha256sum
Checksums matched!
Now using node v9.4.0 (npm v5.6.0)

earlier in the build.

I suspect the logic to determine the exact version of stable is broken. Workaround is to hardcode a 9 version.

pvdlg commented 6 years ago

Per Travis documentation you have to use the keyword "node" to use the latest Node stable version.

node latest stable Node.js release

"stable" would make more sense, but Travis decided to go with the keyword "node" instead it seems. "stable" might also work, but as it's not documented it's probably not officially supported.

For information, it's also mentioned in the README that you can use the keywords node or lts/*:

Your code will run only on the job identified as the build leader, which is determined as follow, by order of priority:

  • The job with the ID defined in the -b, --buildLeaderId CLI options or the buildLeaderId API option or BUILD_LEADER_ID environment variable.
  • The job configured with the latest Node version (node_js: node).
  • The job configured with the lts Node version (node_js: lts/*).
joscha commented 6 years ago

"stable" would make more sense

I think stable is much older than node, this commit is about 2 years old: https://github.com/joscha/gulp-rewrite-css/commit/9aa1da7db6a12710f99b7899331473db2fb6bfd0 so I suspect we might see the error I am describing a lot more once people are adopting travis-deploy-once. From a compatibility perspective I think it would make sense to support both, I am happy to adapt #50 so node trumps stable - what do you think?

joscha commented 6 years ago

stableshows up here in https://github.com/travis-ci/docs-travis-ci-com/commit/e7f3177d6e2efcb4273747871020db4d265d4e42#diff-039683e57475ce0ea17b484a85383075 on 13 Feb 2015. It was deprecated some time later, the only mention it was deprecated I can find here: https://github.com/travis-ci/docs-travis-ci-com/commit/da24ea184f91e38ef3e254c9cc5b9f313775362f#diff-039683e57475ce0ea17b484a85383075 from 26 Mar 2016.

joscha commented 6 years ago

references https://github.com/travis-ci/docs-travis-ci-com/pull/534

pvdlg commented 6 years ago

We recently made the choice to not add iojs as io.js is not developed anymore. With the same reasoning, I'd rather keep things as they are and not bother including stable as it's deprecated.

@gr2m any preference?

I suspect we might see the error I am describing a lot more once people are adopting travis-deploy-once

Do you mean a lot more people will use it because it's mentioned in the semantic-release doc? If yes, it's not really the case because travis-deploy-once was used before within semantic-release itself. We extracted it so we could support other CI more easily, but it as always been there.

Moreover, as soon as Build Stages are out of beta we will recommend them instead of travis-deploy-once. travis-deploy-once will be maintained but probably deprecated, or at least won't see much new features.

joscha commented 6 years ago

I'd rather keep things as they are and not bother including stable as it's deprecated.

I agree that it's deprecated, but if you update semantic-release currently and use travis-deploy-once, your build will fail, even though there is nothing wrong with the configuration. The build leader election doesn't throw, it just selects no leader at all if you have 8 and stable for example (e.g. all builds think they are not the leader). Hence I think we should either support stable or at least throw. I added 7827b74 in #50 to have stable trumped by node, I could add a logger warning there or change #50 to throw if stable is encountered.

Do you mean a lot more people will use it because it's mentioned in the semantic-release doc?

I think it will happen because semantic-release-cli setup adds it as a first-class citizen to the travis file, yes.

pvdlg commented 6 years ago

I agree that it's deprecated, but if you update semantic-release currently and use travis-deploy-once, your build will fail

No, if it was working before it will work now. As explained previously travis-deploy-once has always been used by semantic-release. Before it was called within the code of semantic-release, now it's recommended to be used as a CLI.

The build leader election doesn't throw, it just selects no leader at all if you have 8 and stable for example (e.g. all builds think they are not the leader).

That's not what happens. If you defined stable and 8, then 8 will be defined as the build leader everywhere. As you can see in your build, 8 was defined as the build leader and semantic-release was executed: https://travis-ci.org/joscha/gulp-rewrite-css/jobs/331351874#L593.

As far as I can tell, everything work has expected: stable is ignored and the highest Node version is used as the build leader.

I think it will happen because semantic-release-cli setup adds it as a first-class citizen to the travis file, yes.

As explain above travis-deploy-once as always been used by semantic-release.

joscha commented 6 years ago

No, if it was working before it will work now.

I am not sure that is correct. I was using https://github.com/dmakhno/travis_after_all before this change with an older semantic-release version. I updated semantic-release and removed https://github.com/dmakhno/travis_after_all in favor of travis-deploy-once. If I have came from a build matrix with "stable, 4" for example, then I would expect the build to work. But as semantic-release needs Node 8, selecting Node 4 as the build leader will fail, which is what travis-deploy-once will do.

As far as I can tell, everything work has expected: stable is ignored and the highest Node version is used as the build leader.

That's why I think it's not working as expected. I guess there is a difference in our understanding what "highest Node version" means, because the elected build leader https://travis-ci.org/joscha/gulp-rewrite-css/jobs/331351874 is not the highest node version of that build pair, as it is Node 8. The highest Node version is in https://travis-ci.org/joscha/gulp-rewrite-css/jobs/331351873 (Node 9.4.0 has been activated via nvm there). But I suppose if "stable" is not considered a valid node version (which I don't think is correct), then yes, Node 8 is the "highest Node version" of this pair.

Anyway, for anyone ending on this ticket because their build fails, just replace "stable" with "node" after updating and you'll be 👌.

pvdlg commented 6 years ago

I am not sure that is correct. I was using https://github.com/dmakhno/travis_after_all before this change with an older semantic-release version.

Ah ok! Indeed semantic-release was using travis_after_all a long time ago (way before I joined). It seems we switched to travis-deploy-once about a year and half ago.

That's why I think it's not working as expected. I guess there is a difference in our understanding what "highest Node version" means

I was referring to this quote in your previous comment:

The build leader election doesn't throw, it just selects no leader at all if you have 8 and stable for example (e.g. all builds think they are not the leader)

It's not accurate, with 8 and stable the 8 job is used as the build leader.