search

watson / ci-info

Get details about the current Continuous Integration environment
MIT License
319 stars 49 forks source link

Parity with @npmcli/ci-detect #95

Closed wraithgar closed 1 year ago

wraithgar commented 1 year ago

Greetings from the npm cli team. We recently had a bug filed in our own ci detection library that brought your library back to our attention. After doing some digging it looks like possibly neither of our projects are doing things exactly the same, and this PR is an attempt to get us in sync with the eventual goal being to deprecate our library and start using yours.

In order for us to move to this library withouth it constituting a semver major change in npm itself, we need to at the very least have your library set isPr in the same environments that @npmcli/ci-detect currently does. None of the other metadata really matters as far as npm itself is concerned.

This PR gets us MOST of the way via the following commits. I don't know if you'd rather these all be separate PRs, or if you want the commits squashed and the title to cover all the changes. Whatever works for you works for us. Here are the changes present:

@npmcli/ci-detect currently also checks for Wercker, but that does not appear to be in use anymore so it was not included in this PR.

As far as the three vercel environments vercel-bitbucket, vercel-gitlab, and vercel-github, I have asked some folks at vercel if detecting VERCEL_URL is sufficient for knowing if one is generally in any of those environments, if so isCi is going to be true still and this will mean detection parity with @npmcli/ci-detect

One final side-note. It doesn't appear that the index.d.ts file is coupled in any way to index.js in tests. If I hadn't looked at other PRs I wouldn't even have thought to update it along with new vendor entries.

Looking forward to any feedback you have on this PR. We are pretty excited about the idea of having one less module to maintain so we can focus on package management and not ci detection.

wraithgar commented 1 year ago

One more thing I forgot to mention was that this repo doesn't have an engines field so I wasn't sure what features I could use. Your CI tests down to the latest node 8 so I used that as my floor.

String.includes is compatible from node 4 and up so I assume we're good to go here.

sibiraj-s commented 1 year ago

Thanks @wraithgar . Changes looks good to me but I have one comment though on JENKINS.

One more thing I forgot to mention was that this repo doesn't have an engines field so I wasn't sure what features I could use. Your CI tests down to the latest node 8 so I used that as my floor.

Yeah. the engines field is missing, can be added. The package is simple and not concerned about node, but also didn't want to support very very old non maintained versions either. Plus, CI is never green on those due to devDependencies requirement on the engines, hence 8. But ignoring that it should cover most node versions than mentioned.

Looking forward to any feedback you have on this PR. We are pretty excited about the idea of having one less module to maintain so we can focus on package management and not ci detection

This is great. Awesome 🎉

As far as the three vercel environments vercel-bitbucket, vercel-gitlab, and vercel-github, I have asked some folks at vercel if detecting VERCEL_URL is sufficient for knowing if one is generally in any of those environments, if so isCi is going to be true still and this will mean detection parity with @npmcli/ci-detect

👍

One final side-note. It doesn't appear that the index.d.ts file is coupled in any way to index.js in tests. If I hadn't looked at other PRs I wouldn't even have thought to update it along with new vendor entries.

Yeah. Sometimes I miss them too 😅 . Will see if that could be automated.

wraithgar commented 1 year ago

Thank you both @sibiraj-s and @kanadgupta.

I have amended this PR as per the very helpful feedback y'all gave, including adding an engines field.

sibiraj-s commented 1 year ago

Thanks @wraithgar . Published in v3.6.0 🎉

wraithgar commented 1 year ago

Follow up on the Vercel question, this is mostly for the npm folks tracking the move to this module.

Question:

Is VERCEL_URL set in the environment whenever these are also set? VERCEL_GITHUB_DEPLOYMENT VERCEL_GITLAB_DEPLOYMENT VERCEL_BITBUCKET_DEPLOYMENT

Answer:

If one of those three is set VERCEL_URL is also set. VERCEL_GIT_PROVIDER can be used to consolidate that list. Plain old CI is set in the build env. Its also possible that VERCEL_URL is set and the project is not linked to vcs.

As of now isCI has parity with @npmcli/ci-detect with all providers that still exist.

wraithgar commented 1 year ago

npm@9.1.2 is now using this library instead of @npmcli/ci-detect

Thanks again for everyone who helped w/ this.