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

Reach out to Travis to get option that includes #205

Closed
mhdawson opened this issue May 13, 2019 · 48 comments
Closed

Reach out to Travis to get option that includes #205

mhdawson opened this issue May 13, 2019 · 48 comments
Assignees
Labels
enhancement New feature or request vendor Some vendor need to be onboard

Comments

@mhdawson
Copy link
Member

LTS releases + current.

@mhdawson mhdawson assigned ljharb and Eomm and unassigned Eomm May 13, 2019
@ljharb
Copy link
Member

ljharb commented May 13, 2019

This is already doable by adding matrix lines in .travis.yml for each LTS signifier that nvm supports, as well as node. I'm not sure how it would work to have something in the yml that created more than one item.

@dominykas
Copy link
Member

dominykas commented May 14, 2019

This is already doable by adding matrix lines

Yes, it is possible to do this and we should evangelize that, however I think this issue is to change the default, i.e. the behavior when you have no versions defined. At the moment it will only run in the latest stable - I think we should advocate that it should run in the latest stable + latest LTS and that is without adding any lines other than language: node_js in the .travis.yml.

I also think that lts/* was intended to support multiple LTS versions, but maybe they never got around to implementing that.

So, to summarize, what I'd really love to see travis do is support the following:

  • node latest stable Node.js release (same as they currently)
  • lts/active all active LTS Node.js releases (there is no such option right now)
  • default: current (or active or smth like that) equivalent of having both node and lts/active in the version list (currently: default is node).

Maintainers can then chose to stick to the default (ideally) or override to only test in one or more specific lts/latest/whatever they feel like.

Docs: https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#specifying-nodejs-versions

Edit: update to reflect @ljharb's commend on the intent of lts/*.

@ljharb
Copy link
Member

ljharb commented May 14, 2019

I’m the maintainer of nvm who invented “lts/*” - in no way was it ever intended to support multiple versions, it’s a single star and behaves the same way a single star does on the command line, selecting the first matched version only.

@dominykas
Copy link
Member

dominykas commented May 14, 2019

Cool, thanks, good to know! Did not realize this comes from nvm and assumed it's a glob.

Updated to suggest lts/active instead.

@ljharb
Copy link
Member

ljharb commented May 14, 2019

That would still indicate a single version; nvm (travis’ sole means of deciding node version) does not have any concept of selecting multiple versions, only listing them.

I can certainly ask Travis to change the default matrix to have multiple jobs, but i anticipate pushback about the complexity for users of getting, say, 3 jobs by default, adding 1, and ending up with 1 - if that comes up, how should i respond?

@dominykas
Copy link
Member

dominykas commented May 14, 2019

Ideally, library maintainers, esp. for popular libs, should be testing at least in all active LTS plus stable, but if that's not feasible (there's an obvious cost associated with 3x the job runs), then the current default of stable is a good one, I would say.

Regardless of that, adding a way to define "all active LTS" in a single line of .travis.yml, which does not need to be maintained, would be an awesome feature.

And even without that feature, possibly Travis could update their docs to highlight the current behavior (that people might not be testing in LTS by default) and include a recommendation with an example of a manual configuration. I'm happy to try and work on a proposal for the docs (not sure if they have them in the open so people can PR).

@mhdawson
Copy link
Member Author

I think the

which does not need to be maintained,

part would really be helpful. @ljharb if a single entry mapping to multiple entries does not fit the model any other ideas? I wonder about something like LTS-latest, LTS-1, LTS-2 or some other way to try and catch all of the active LTS versions which would not have to be updated for each new release.

@ljharb
Copy link
Member

ljharb commented May 18, 2019

hmm, probably the easiest way would be for me to add a similar feature to nvm; then travis can get it by default.

@dominykas
Copy link
Member

@ljharb any chance there's an update on this? I'd be keen to hear any and all opinions from Travis, before I start drafting #204... Can I be of any assistance on the nvm side of things (a stretch, given I'm no bash guru, but still)?

@dominykas
Copy link
Member

By the way, renovate does have some syntax on this: https://renovatebot.com/node/

@ljharb
Copy link
Member

ljharb commented Jun 3, 2019

@dominykas the more i think about it, the more i don't think it's really compatible with travis without being built into nvm. (the renovate link 404s for me)

@dominykas
Copy link
Member

Looks like a bug in renovate docs routing or smth, here's a link that works: https://renovatebot.com/docs/node/

@ghinks
Copy link
Contributor

ghinks commented Jun 9, 2019

I left a write up at 206 and the renovate bot does appear to meet our needs.

@dominykas
Copy link
Member

Travis team page: https://about.travis-ci.com/team/

Not sure whether it's appropriate to tag people that are also on GitHub directly into this issue :)

@dominykas
Copy link
Member

Just got a word from Travis that they're changing the default to be the LTS: travis-ci/travis-build#1747 (this ofc means that most packages won't be testing with the next LTS, so there's still work to be done and evangelized).

@mhdawson
Copy link
Member Author

From my reading I think lts/* is only the latest LTS and we were thinking a default for all current LTS releases plus the current release would be what we'd want to be suggesting.

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

I believe that would be a massive architectural change for travis to have a default matrix instead of a single default job.

@mhdawson
Copy link
Member Author

mhdawson commented Jul 25, 2019

Sorry for for the confusion, should have re-read the issue to refresh my memory. I can see that the default being to test all LTS versions and current could be impractical, but a way that it could be selected easily without having to update it every time a new LTS release comes out would still be useful. Sounds like the easiest way would still be an option in nvm?

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

lts/* always live-updates (on any remote command: ls-remote and install, eg) to whatever the latest LTS is based on nodejs.org/dist/index.tab

@mhdawson
Copy link
Member Author

Right so that covers the latest, but we'd want something similar to the previous LTS version as well as there is usually 2 active LTS releases, if not 3.

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

nvm could certainly add lts/*-2 or something similar, but that still wouldn't make it practical to have travis default to testing more than one node version.

@mhdawson
Copy link
Member Author

But it would make it easier for a maintainer to setup their testing to test all LTS versions and Current once and then not have to update it for each LTS release right?

@ljharb
Copy link
Member

ljharb commented Jul 25, 2019

If they hardcoded the assumption that there's 2 active LTS lines, then yes.

@wesleytodd
Copy link
Member

wesleytodd commented Aug 8, 2019

With the new announcement of Gihub CI, we might also want to reach out to them to get the same "default" behavior for node environments we are working on for Travis. I am not exactly sure how it will work, but it looks like there will be some built in steps you can leverage (from the landing page see below). If we can have some say in how these work it would set a good precedent for all the users who will probably jump on board with this.

 - uses: actions/setup-node@master
      with:
        version: 12

EDIT: I opened this issue: actions/setup-node#25

@wesleytodd
Copy link
Member

wesleytodd commented Aug 9, 2019

An issue to track the aliases they will add. Probably a good place to get our opinions heard: actions/setup-node#26

EDIT: also opened up a PR on the templates they provide to use all current majors as the default. actions/starter-workflows#19

@chrispat
Copy link

@wesleytodd I have not followed this thread in detail but would be curious to understand what recommendations the node community has. The starter workflow for node.js is in the actions/starter-workflows repo. We would welcome any pull request for suggestions to improve it.

@wesleytodd
Copy link
Member

wesleytodd commented Aug 12, 2019

Hi @chrispat, the main goal is to improve the story around package authors testing their packages in all of the current node versions. It is very typical for authors to set travis to test the current versions at the time of publishing, but that is quickly out of date. The PR I made above is just to get users started, but what we really need is an alias. It sounds like, from the people who commented on those issues I linked above, this is on the roadmap. So I don't know if there is more to do at this time from the GH side.

@dominykas
Copy link
Member

@chrispat I will be writing a blog post on what people should be testing in, but in short, I think the recommendation is to at least test in the latest version of the current LTS release lines (currently: 8.16.0 and 10.16.2), but ideally to test in all the actively maintained release lines (currently: 8.16.0, 10.16.2, 12.8.0; in Apr/2019: latest versions of 6.x, 8.x, 10.x, 11.x).

Ideally these lists would be available via some alias, so that people do not have to keep that updated. There is a discussion in #236 on what keywords to use for such aliases.

@Eomm Eomm added enhancement New feature or request vendor Some vendor need to be onboard labels Aug 31, 2019
@mhdawson mhdawson removed the package-maintenance-agenda Agenda items for package-maintenance team label Sep 10, 2019
@mhdawson
Copy link
Member Author

mhdawson commented Nov 5, 2019

Waiting on

  • Issue in nvm to provide option for oldest LTS release
  • Issue in Travis to adopt the above
  • (blog post already complete)

@ljharb
Copy link
Member

ljharb commented Nov 14, 2019

This can now be achieved trivially by using my shared configs: https://twitter.com/ljharb/status/1194728750698487808

iow, anyone who uses my shared travis configs will get new LTS versions automatically, since i add new node versions to that repo pretty rapidly (and eventually could automate it with github actions).

@dominykas
Copy link
Member

dominykas commented Nov 15, 2019

👍 This is great!

What about deprecating old versions? Oh, I see - it's dependent on the include. Nice.

@ljharb
Copy link
Member

ljharb commented Nov 15, 2019

Happy to add any additional composable "presets" to that shared repo as folks need it.

@mhdawson
Copy link
Member Author

Very nice !

@ljharb
Copy link
Member

ljharb commented Nov 22, 2019

as an example, node v13.2 just was released, so now every project using the "minors" presets will automatically test on it also (as opposed to instead of 13.1), on their next travis run.

@dominykas
Copy link
Member

  1. Should we have this under @pkgjs? I know @ljharb is not some rando on the internet, but if we were to make an official recommendation - should that be in an official repo?
  2. It does a bit more than just include the relevant versions (e.g. npm run tests-only support)? Do we want to be that opinionated? I think I'd favor something that has just the basics.
  3. It would probably make sense to provide includes named based on the keyword convention we have for package support?

@mhdawson
Copy link
Member Author

mhdawson commented Nov 27, 2019

I'd be +1 for having it under @pkgjs

@ljharb
Copy link
Member

ljharb commented Nov 27, 2019

I'd be happy to fork it into an official org, and then keep both remotes updated at the same time.

@ljharb
Copy link
Member

ljharb commented Nov 27, 2019

As for the tests-only thing, that's kind of necessary if we want to separate linting, from auditing, from tests - since npm test is supposed to always run 100% of your tests, I've chosen "pretest", "posttest", and "tests-only" as the convention. There always has to be some convention since there's no way to do npm test --ignore-scripts, unfortunately.

@dominykas
Copy link
Member

dominykas commented Nov 27, 2019

If I understand it correctly, Travis can import just the nodejs bit - it doesn't need to have the script, env, etc? Similarly, while I personally also prefer to have nvm install-latest-npm in my CI, I would not say that this is the 100% default for everyone?

@ljharb
Copy link
Member

ljharb commented Nov 27, 2019

That's a fair point; I can separate out the matrices, and the "latest npm", to separate imports, to allow people to pick and choose those and define their own script/pretest/posttest settings.

Separately tho, I'm not sure what use case anyone has in CI for not testing with the latest npm :-)

@dominykas
Copy link
Member

Separately tho, I'm not sure what use case anyone has in CI for not testing with the latest npm :-)

I can come up with several edge cases, alright... Not sure if they're relevant for people publishing to public npm, though. One example is doing stuff with docker - the base images have a fixed npm which is distributed with the node release. Then there's occasional edge cases for more complex situations with peer deps and/or shrinkwraps, which are about to get breaking changes and ever so often have regressions. Then there's the logic of using the same npm your users are likely to use (i.e. default one bundled with node - although I'd love to see some stats on this!)

Anyways, like I said - I'm with you on this one ;) just not sure if we want to make it the default.

Note to self: we should update best practices to include this, if they don't already do that.

@ljharb
Copy link
Member

ljharb commented Nov 28, 2019

I’ll comment here once I’ve separated out the unopinionated parts.

@dominykas
Copy link
Member

We now have https://github.com/nodejs/ci-config-travis - if there's no objections, I'll close this issue in a week or so?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vendor Some vendor need to be onboard
Projects
None yet
Development

No branches or pull requests

8 participants