Create ci-cd-guidelines.md #146
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| ## What | ||
|
|
||
| Guidelines for CI/CD automation templates to be shipped by the package-maintenance team. | ||
|
|
||
| > **Note:** This set of guidelines _is not_ about telling projects what they can and cannot do. Instead, it's a framework for those who are interested in building out bare-minium CI/CD templates for projects in the JavaScript ecosystem to use. | ||
|
|
||
| ## Why | ||
|
|
||
| Provide a baseline, common set of CI/CD infrastructure that can be relied upon regardless of what CI/CD providers are used by a project. | ||
|
|
||
| ## How | ||
|
|
||
| These guidelines are intended to be just that – guidelines. We will also include templates for common CI/CD providers and accept contributions of templates that implement these guidelines from the community. | ||
|
|
||
| It's worth noting that projects **do not** need to use one of the provided templates, but **do** need to ensure that their CI/CD setup enables them to follow the required guidelines at the very minimum. Projects are by no means restricted to the required guidelines – more complex and dynamic configurations are more than welcome. | ||
|
|
||
| ## Definitions | ||
|
|
||
| Definitions for the various terms used in the guidelines: | ||
|
|
||
| ### Required | ||
|
|
||
| To say a CI/CD template follows the "tests" guidelines documented here, it would need to meet all of the **required** marks. The same approach applies to for all other guidelines. | ||
|
|
||
| ### Optional | ||
|
|
||
| **Optional** guidelines are, as the name indicates, optional for templates to include. They're _nice to have_ but by no means required. | ||
|
|
||
| ## CI/CD Guidelines | ||
|
|
||
| - Tests | ||
| - **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. | ||
| - `npm ls` | ||
| - If `npm ls` exits with a `0` exit code, your build should pass | ||
| - If `npm ls` exits with a non-`0` exit code, your dependency graph is invalid and should fail the run | ||
| - **Optional**: | ||
| - code coverage | ||
|
Member
There was a problem hiding this comment. Could we add also |
||
| - failure if code coverage is below a well-documented threshold | ||
|
Member
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. @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.
Contributor
Author
There was a problem hiding this comment. Would y'all like me to change/remove this? There was a problem hiding this comment. 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 There was a problem hiding this comment. we sit at
Member
There was a problem hiding this comment. While I'd say coverage is good for every project, the decision to use it imo should be made project by project.
Contributor
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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? There was a problem hiding this comment. 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:
Contributor
Author
There was a problem hiding this comment. 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" 😁 |
||
| - Operating System Versions | ||
| - **Required**: | ||
| - Windows | ||
| - macOS | ||
|
Member
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. travis-ci has both mac and windows support, included in the free plan.
Member
There was a problem hiding this comment. WIndows support in Travis and Appveyor is not at the same level of the one in Azure. 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.
Member
There was a problem hiding this comment. 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.
Member
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. 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 💖 There was a problem hiding this comment. 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 |
||
| - Linux | ||
| - All active LTS releases of Ubuntu or your CI provider's default Linux OS | ||
| - Node.js Versions | ||
| - **Required**: | ||
| - nodejs@current | ||
| - nodejs@lts | ||
| - This includes all active and maintenance versions | ||
| - **Optional**: | ||
| - nodejs@nightly | ||
|
Member
There was a problem hiding this comment. 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.
Contributor
Author
There was a problem hiding this comment. Agreed, happy to remove this if others agree 👍
Member
There was a problem hiding this comment. I would love to add this this to some packages, but there is really no docs. Maybe we can just add some somewhere else?
Contributor
Author
There was a problem hiding this comment. Since this is optional, should this be removed given @mcollina's comment?
Contributor
Author
There was a problem hiding this comment. 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. |
||
There was a problem hiding this comment.
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.