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

Create ci-cd-guidelines.md #146

Merged
merged 4 commits into from
Jun 14, 2019
Merged

Create ci-cd-guidelines.md #146

merged 4 commits into from
Jun 14, 2019

Conversation

bnb
Copy link
Contributor

@bnb bnb commented Jan 31, 2019

An initial proposal for ci-cd-automation-guidelines.md, which outlines bare minimum set of requirements around CI/CD templates.

An initial proposal for `ci-cd-automation-guidelines.md`, which outlines bare minimum set of requirements around CI/CD templates.
- nodejs@lts
- This includes all active and maintenance versions
- Criteria (**Optional**):
- nodejs@nightly
Copy link
Member

Choose a reason for hiding this comment

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

i don't think it's common or even a good idea for most packages to test on nightly; nightlies are for developers on node core, not for ecosystem users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, happy to remove this if others agree 👍

Copy link
Member

Choose a reason for hiding this comment

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

I would love to add this this to some packages, but there is really no docs. Maybe we can just add some somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this is optional, should this be removed given @mcollina's comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we should leave this as optional, given it's a guideline for templates to be submitted to be included in this repo rather than a list of requirements for projects to be included.

- Many CI/CD systems run this implicitly for JavaScript projects. You do not need to explicitly define `npm test` in these cases.
- Criteria (**Optional**):
- code coverage
- failure if code coverage is below a well-documented threshold
Copy link
Member

Choose a reason for hiding this comment

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

Coverage reports are good, but I personally am not a fan of failing. Code coverage is a notoriously poor indicator of good testing, and suggesting failure on as coverage threshold is a reinforcement of the problem.

I don't know of tooling that does this, but it would be great if you could monitor if it drops significantly (by percentage) from one commit to another. If that existed I would be more willing to recommend warning or failing.

Copy link
Member

Choose a reason for hiding this comment

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

@bcoe just added an option to C8 which allows a fail below a certain level. We plan to use this in the community regression runs as a "sanity" check, setting it well below the current level. It should also be perfectly acceptable to lower that threshold in a PR as we know coverage goes up/down as new features etc go in. I see the threshold as a good canary and can be used to achieve what @wesleytodd is suggesting. Set a threshold so you know if there is a large drop from the last known good, but allow the threshold to be changed if that drop is ok and understood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would y'all like me to change/remove this?

Copy link

Choose a reason for hiding this comment

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

As @mhdawson says, the goal isn't to enforce that a pull request has coverage, the goal is to fail if we break the coverage machinery. A pull request should never drop coverage from 95% to 55%, this is most likely a change to Node.js that breaks the instrumentation mechanism.

Copy link

Choose a reason for hiding this comment

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

we sit at 95% coverage today, if we set the threshold to 85% there's no healthy scenario that would cause this significant of a drop.

Copy link
Member

Choose a reason for hiding this comment

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

While I'd say coverage is good for every project, the decision to use it imo should be made project by project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although I'm a bit of a testing fan, I have just recently been reminded that some modules ( like the N-API conversion type ones for hardware etc) are not testable in the modern CICD sense. I am with @ljharb in that coverage is good. But in some cases like add on native code they are quite impossible without bespoke hardware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The consensus here seems to be split – it's good in some cases. Since this is in the optional section, can it remain or should it be removed?

Copy link

Choose a reason for hiding this comment

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

I think that coverage is almost always good, as @dominykas puts it: "use it to find untested code in a module, not to prove code is good".

If someone is using a static typing tool (like TypeScript) or a smart linter (like standard) this can certainly solve some of the same variety of issues that you solve with test coverage -- I might group all of these types of tools under the category of optional, with a discussion of why they're potentially great indicators of a healthy codebase:

  • find un-used variables.
  • find incorrectly invoked function signatures.
  • find code that just straight up has never been run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For what it's worth, I think this should remain as an optional criteria for CI/CD templates to be PRed into this repository, given that there seems to be an even split on the discussion – some find it valuable and some don't. Perfect fit for "optional" 😁

@ljharb
Copy link
Member

ljharb commented Feb 1, 2019

One thing that’s very important, but not enough packages do, is running npm ls in CI to ensure their dependency graph is valid. I’d like to include that here.

@bnb
Copy link
Contributor Author

bnb commented Feb 1, 2019

@ljharb totally fine with adding that. A couple questions:

  1. Would you put this under testing or another category?
  2. Is there an additionally needed step outside of npm ls to define that the tree is indeed returning what is expected?

@ljharb
Copy link
Member

ljharb commented Feb 1, 2019

I'd put it under testing, since it's ensuring that your package.json changes are valid.

No, after npm install, either npm ls exits zero, or your dep graph is invalid and you can't rely on anything working. This is particularly important in any ecosystem that uses peer deps, which includes babel, eslint, react, angular, vue, jest, gulp, grunt, etc.

@wesleytodd
Copy link
Member

wesleytodd commented Feb 1, 2019

@ljharb Instead of npm ls couldn't we recommend running npm ci which will ensure an install compatible with the package-lock.json? Isn't this the same effect that it validates the install?

@ljharb
Copy link
Member

ljharb commented Feb 2, 2019

@wesleytodd no, as it doesn’t validate anything afaik. You can easily have a lockfile with invalid deps.

Separately, that requires a lockfile, and i feel very strongly that a lockfile on a published project is a very bad practice - only apps should have lockfiles.

@wesleytodd
Copy link
Member

Didn't know it did not validate., good to know! As for having a lock file on packages I get the point, I don't always follow the advice but feel like I should. Maybe having a section on how to disable it is good as well.

- Operating System Versions
- Criteria (**Required**):
- Windows
- macOS
Copy link
Member

Choose a reason for hiding this comment

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

I don’t think Windows or MacOS should be required. This favors significantly one Cloud vendor vs another. Linux is required. Windows and Mac recommended.

Copy link
Member

Choose a reason for hiding this comment

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

I think considering only cloud scenarios is not enough - node and it's package ecosystem is heavily used for frontend and lots of developers work on Win/Mac, so it is very important for packages to behave well on the dev envs, not just on deployed instances.

That said, I do realize that maintaining compat is extra work (and hard work), esp on Win, esp if you only own a Mac. That is probably a broader topic to discuss for this group, as there's scope to help maintainers with that.

Copy link
Member

Choose a reason for hiding this comment

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

This initiative is part of the Node Foundation. We cannot require developers to use a single CI provider (Azure) as all other alternatives are significantly worse.

Copy link
Member

Choose a reason for hiding this comment

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

travis-ci has both mac and windows support, included in the free plan.

Copy link
Member

Choose a reason for hiding this comment

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

WIndows support in Travis and Appveyor is not at the same level of the one in Azure.
The time required for a build takes so much, and I had some issues with Travis on Windows (failed build for no reason, broken output etc). Overall it's a huge requirement.

Unless it's a very specific case (a CLI utility, something that deals with the filesystem), testing on Windows and/or Mac only ensure that a contributor can use those system to send a PR. This is valuable, but less so.

Along the same lines, we could argue that a package must be tested installing it with yarn as well. This is more valuable in a lot of cases, as a some differences in the resolution algorithm could potentially break the package.

A huge amount of downloaded packages on npm do not do either of this, because it's a huge effort.

Copy link
Member

Choose a reason for hiding this comment

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

i don’t agree that yarn matters in any way whatsoever since node doesn’t officially support yarn, while it does officially support macs and windows - the definition for what’s correct for yarn is “what npm does”, so if it’s tested on npm its already good enough for yarn.

However, the difference between OSs only matters to a tiny minority of packages, so I’d be in support of only mentioning Linux here for the common case.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should clearly focus on supporting only the things that node supports. This is not to say what different communities are doing is bad or not important. It is just that the scope becomes to large to realistically support best practices. We can always expand once we nail down the basics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I'd agree with @wesleytodd in expressing that we should support what Node.js supports, and help do the work to lower the barrier to entry for OSS projects to do the same.

My intent here is not to favor one CI provider over another – I've had personal experience only with TravisCI's Windows builds and had very few problems with them other than the Node.js version manager used by Travis failing once. I wasn't aware of the general state of poor support in the CI/CD ecosystem for Windows builds, which deeply saddens me since a non-trivial amount of the downloads from nodejs.org are for Windows.

For context on why this was an interesting and exciting inclusion to me, I started learning and eventually contributing to Node.js on a $200 PC that I purchased at WalMart. I've told multiple contributors and collaborators about the experience I had and often asked what I could to do help prevent others from having that experience.

That experience was horrid because Windows support was so dramatically lacking at the time. While things have gotten better both in Node.js core and in the ecosystem, I still always want to do whatever I can to help make sure the next person who uses Node.js on Windows doesn't have anything that resembles the same experience that I did when I started.

That said, I am going to remove myself from this specific discussion point since @mcollina suggested that it prioritizes one provider – the one I am currently employed by – over any other. That is expressly not my intent. If y'all could seek consensus on this point, I (or anyone else with commit access) can update it accordingly 💖

Copy link

@vweevers vweevers Mar 8, 2019

Choose a reason for hiding this comment

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

I do not share the view that the current text favors a single provider (which is not even mentioned) supposedly because "other alternatives are significantly worse". Personally I'm happy with AppVeyor (their build times have significantly improved) and Travis for Windows is promising. Before this thread, I never even considered Azure as an option.

That said, I agree that Windows should not be required (except for native addons). For most packages it indeed adds no value, provided that other best practices are followed, e.g. use path when dealing with filenames. Could that be worth a separate guide? Edit: I came across portable-node-guide, exactly what I had in mind.

- nodejs@lts
- This includes all active and maintenance versions
- Criteria (**Optional**):
- nodejs@nightly
Copy link
Member

Choose a reason for hiding this comment

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

I would love to add this this to some packages, but there is really no docs. Maybe we can just add some somewhere else?

@dominykas
Copy link
Member

Is covering some more options of the release process (eg semantic-release, 2fa for publidh etc) in scope for this document?

@mcollina
Copy link
Member

mcollina commented Feb 2, 2019

I think we should have a paragraph on 2FA.

@mcollina
Copy link
Member

mcollina commented Feb 2, 2019

I think “flaky” CI is a significant burden to maintainers. I did this wrong several times, and it’s almosr ab unrecoverable situation. Helping out with getting a solid green would be extremely helpful.

@bnb
Copy link
Contributor Author

bnb commented Feb 6, 2019

@mcollina how would you like that to be encapsulated in a bullet point?

@Eomm
Copy link
Member

Eomm commented Feb 8, 2019

@ljharb Instead of npm ls couldn't we recommend running npm ci which will ensure an install compatible with the package-lock.json? Isn't this the same effect that it validates the install?

Thinking on the modules, the package-lock.json cannot be published in the npm registry, so could it be more user-friendly not run npm ci in CI/CD? So the module will be tested with the dep valid in that moment.

@mcollina
Copy link
Member

mcollina commented Feb 8, 2019

@bnb

@mcollina how would you like that to be encapsulated in a bullet point?

  • The CI run should be green on repeated runs. Tests that fails intermittently are problematic, and a burden for maintainers.

@ljharb @Eomm

I think that package-lock.json creates more harm than good in maintaining packages. I'm not alone in this opinion. It might be good to document this choice, with pros & cons, or just avoid mentioning it.

@nschonni
Copy link
Member

nschonni commented Feb 8, 2019

could it be more user-friendly not run npm ci in CI/CD

The command is available in npm 5.7.1, so it might not work for everyones matrix, depending on what Node/NPM versions they're supporting. Think once 6 drops off LTS, all the latest LTS will meet this minimum requirement though

@ljharb
Copy link
Member

ljharb commented Feb 8, 2019

@nschonni yes, but supporting only LTS isn't necessarily a good thing, and as has been said, having a lockfile (needed for npm ci) is not necessarily a good thing for packages.

- This can be any testing framework exposed via `npm test`.
- Many CI/CD systems run this implicitly for JavaScript projects. You do not need to explicitly define `npm test` in these cases.
- Criteria (**Optional**):
- code coverage
Copy link
Member

Choose a reason for hiding this comment

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

Could we add also vulnerability scanner as optional criteria?
I saw in action today and I think could be a good alert for the maintainer

@mhdawson
Copy link
Member

@bnb I think the next step is an update to address remaining comments?

@bnb
Copy link
Contributor Author

bnb commented Mar 8, 2019

@mhdawson yup! Will take a pass now. Apologies for the delay, been busy the last few weeks 👍

@mhdawson
Copy link
Member

@bnb thanks :)

@bnb
Copy link
Contributor Author

bnb commented Mar 15, 2019

@mhdawson sorry for the delay! Had the changes done locally since my last comment but didn't commit + push. PTAL ✨

@bnb
Copy link
Contributor Author

bnb commented Mar 15, 2019

Also, it is worth a reminder: this is not for the CI/CD of any individual project, which does seem to be what some of the comments have been geared toward – this is for generalized CI/CD templates to be included in this project as a resource for project maintainers to use.

@bnb
Copy link
Contributor Author

bnb commented Mar 15, 2019

@mcollina on the maintaining a green CI, I'm not sure how that actually fully fits into guidelines for CI/CD templates. What code could be in a CI/CD template (for example, .travis.yml) that could point to this?

That's more of a per-project criteria that is geared toward acceptance rather than CI/CD templates 👍

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Please remove the nightly bullet point.


- Tests
- Criteria (**Required**):
- **Required**:
- `npm test`
- This can be any testing framework exposed via `npm test`.
- Many CI/CD systems run this implicitly for JavaScript projects. You do not need to explicitly define `npm test` in these cases.
Copy link
Member

Choose a reason for hiding this comment

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

to clarify, your package.json needs to, but your CI config might not.

Copy link
Member

@Eomm Eomm 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 this draft should land.

There is a pending comment about vulnerability scanner, but we could think in a separate follow up where talk about security

@Eomm
Copy link
Member

Eomm commented May 19, 2019

Hi @bnb, can I help you to address comments in order to complete this PR?

Copy link
Contributor

@ghinks ghinks 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 a great deal of conversation has taken place upon this topic and that these guidelines are in good shape. There are so many single issues here such as 100% coverage that have no absolute answers. These are guidelines and I think as we adopt packages we will adjust them.

@Eomm
Copy link
Member

Eomm commented Jun 11, 2019

@ghinks I agree and then starting from this draft we can build CI/CD templates in order to verify all those issues that are not clear

@mhdawson
Copy link
Member

I agree with @Eomm and @ghinks lets land this as a draft and then we can continue to refine through PRs'.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Eomm Eomm merged commit f3fb160 into master Jun 14, 2019
@ljharb ljharb deleted the bnb/ci-cd-guidelines branch June 14, 2019 22:57
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