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

RFC: Add staging workflow for CI and human interoperability #92

Closed
wants to merge 2 commits into from

Conversation

djsauble
Copy link

See the RFC

@djsauble djsauble added Agenda will be discussed at the Open RFC call Needs Discussion is pending a discussion labels Jan 22, 2020
@dominykas
Copy link

Sorry, maybe I missed something, I know there's the RFC meetings, but can't find the calendar for them - when is this going to be discussed? Is it better to wait for the meeting or post the questions now?

@isaacs
Copy link
Contributor

isaacs commented Feb 14, 2020

The registry will support a data structure like this:

{
  "stagedVersions": {
    "whatever": "other",
    "metadata": "we wanna",
    "put": "here",
    "versions": {
      "1.2.3": { "manifest": "data..." },
      "1.2.4": { "another": "staged manifest..." }
    }
  }
}

Discussion still to be had about what the dist field ought to look like in staged version manifests.

@ljharb
Copy link
Contributor

ljharb commented Feb 14, 2020

Ideally there'd be more information than just "version", "package.json", and "tarball" - git sha, in particular, would be helpful?

@wesleytodd
Copy link

wesleytodd commented Feb 15, 2020

On the call we discussed the workflows around the tarballs and the package locks. The concern was that if you stage a package and then a user installs from the staging it would mean their tarball location moved and then they would have to do an update of their package lock after the package is promoted.

The reason I was concerned about this is because we talked about installing from the staged location, and I imagined that this would enable beta testers and other similar workflows. This was a mistake. Any workflow which relies on testing from the staging area should been done with pre-release versions and dist-tags.

I have changed my thoughts here and I think as this feature gets rolled out we should make sure it is clear that is not a supported use case. I would like to propose two additions:

  1. The --stage flag on install only works when installing a single package
  2. Installing from --stage is not reflected in the package.json or package-lock.json

I think these would help prevent accidental usage of this feature in a way which I had originally imagined which had problematic DX. You would still be able to run something like a CI test by running $ npm i --stage pkg, but it would not persist across installs or be locked accidentally.

@ljharb
Copy link
Contributor

ljharb commented Feb 15, 2020

I don’t think it should be possible for anyone but an owner to install a staged build; and ideally with the restrictions Wes outlined.

@wesleytodd
Copy link

I thought about adding that, but I wonder if there is reason to let un-authed CI servers install it? So I could set my publish CI pipeline to look for a staged version to test against but let it do so unauthed so I didn't need a CI token for that part.

@ljharb
Copy link
Contributor

ljharb commented Feb 16, 2020

If unauthed people can install a staged version, they can depend on it, and staging serves no purpose.

@wesleytodd
Copy link

They can depend on it by going strongly out of their way if we add the package.json changes. Seems fine to me if they would have to do npm i --stage pkg every build separate from npm i. But then this makes me thing we should for sure also specify that once promoted the --stage version tarball is no longer available.

* Webhook support
* Metadata related to fitness of staged packages
* Multiple staged packages (current spec has a limit of one, successive stages
clobber the previous staged version).

Choose a reason for hiding this comment

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

A little unclear - in the context of this RFC, the "package" means essentially the "specific version of a package"?

This might need clarification, to rephrase that the limit applies to each package version, and the previous staged tarball will not be kept.

* A new `npm promote` command that allows a user to promote a staged package.
* The ability to set a `staging` scope on auth tokens so we can grant read or
publish access to staging.
* A new user permission that allows us to grant a user the ability to read or

Choose a reason for hiding this comment

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

It is a little unclear on how will this work with 2FA. At the moment, the users can set their 2FA to be "auth-only" or always, but there's no in-between mode - how would that work with --stage?

Choose a reason for hiding this comment

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

In the call, it was clarified that there'd be an option of a token with r/w permission without 2FA specifically for staging.

Copy link
Contributor

Choose a reason for hiding this comment

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

As this is 100% determined by the registry, there'll be a subsequent discussion/RFC around adding token permissions and TFA requirements other than login/write.

The CLI is agnostic to such things. It just says "this is who I am" and "this is what I want to do", and it's up to the registry to decide whether to allow it, prompt for a TFA token, or reject it.

For the time being, given the status quo on the registry side, promoting a package requires the same permissions as publishing. But there's no reason why it necessarily has to stay that way forever, and we all expect to change it, it's just outside the scope of this RFC.

publish access to staging.
* A new user permission that allows us to grant a user the ability to read or
promote packages in staging.
* These are separate from the existing read and publish permissions, since we

Choose a reason for hiding this comment

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

Is this strictly necessary for the MVP?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't think it is.

have been staged and not promoted.
* A new `--stage` flag for the `npm publish` command that allows a package to be
published to staging.
* A new `--stage` flag for the `npm install` command that allows a package to be

Choose a reason for hiding this comment

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

It might be worth clarifying here that semver rules still apply (i.e. the staged version, if any, will only be installed if it matches the specifier in package.json)

Copy link

@wesleytodd wesleytodd Feb 17, 2020

Choose a reason for hiding this comment

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

Now that I am reading this, I feel like one other addition should be made here. Install --stage would only work with a single package and a specific version. Example:

Good:

$ npm i --stage foo@1.0.0

Bad:

$ npm i --stage
$ npm i --stage foo
$ npm i --stage foo@^1.0.0
$ npm i --stage foo@latest

Again with the goal that installing from the stage be hard and a very explicit action.

EDIT: read from top to bottom and I see @isaacs has some other ideas. I will have time later today to read them, but it looks like this comment might have been premature.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would npm i --stage foo@1.0.0 do to package.json? A use case for this feature would be to stage an update from a monorepo then post a PR against something outside the monorepo to have Travis-CI test it using the staged update before I promoting.

@dominykas
Copy link

Some things we also discussed in the call, that should probably make it into the RFC (sorry for duplicating stuff already being discussed, just making sure we've got everything covered):

  • for audit purposes, it should be possible to see both users - the one who staged the package and the one who promoted it
    • in the meeting we discussed that npm_user is the person who published, and a new field needs to be added for the person who promotes the package
  • what happens with the lock files
    • can they remain unchanged after the package is promoted? I think we discussed that the answer to this one is probably "yes", i.e. if the client detects that the version was staged, but no longer is, and the integrity matches, then there is no need to modify the lock file?
    • if a different tarball is staged at the same version, then install needs to fail
    • is there a need for a new flag in the lock file or should the fact the tarball is staged be detectable from the URL or smth?
    • if you have a staged package (either by an explicit flag or detected via URL) in the lock file - is it ok to install without the --stage flag?
  • what happens with the tarball URLs
    • does the URL change after the package is promoted?
    • can the URLs be aliased?
    • are the tarballs at the staged URL mutable?
    • caching is also relevant in the context of this discussion (for people using various proxies and replicating registries)

Mike also suggested we post the use cases for this feature. For me, right now, the only use case I do care about a lot is to have an area where I can put a tarball without 2FA, and then promote that tarball with 2FA. I don't particularly care if that tarball is installable, TBH - and maybe that's also an option for the MVP, just so we have something to start playing with?

@dominykas
Copy link

Installing from --stage is not reflected in the package.json or package-lock.json

Not sure that's possible, though? If package.json is not updated, then it will retain the semver range which might not match the staged version? Would you have to update the package.json beforehand? This would then be broken completely as npm i would never work?

I like where you're going with the --stage flag being required and only working explicitly with the package name - in that case it does make sense to not update the package-lock.json, but I wonder if that might not create even more confusion?

Would a restriction of --stage name not introduce issues with deduplication and/or transient deps? Not that these can't be solved, just pointing out the possible implications.

I also like the idea of only allowing the authorized people to use the staged version - if this can be done with the initial version, maybe we don't have to solve all of the tarball URL/package-lock problems, as the usage of the feature would be limited and could feed back into the ultimate decisions?

@ljharb
Copy link
Contributor

ljharb commented Feb 17, 2020

I don’t think a staged version should ever be allowed to exist in a lock file, regardless of who dan install it.

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2020

If you can install it, it belongs in a lockfile. There's no reason why we should not lock it once installed, as that can only result in build irregularities. And if you can't install it, why are we staging it?

Even if staging requires that someone go absurdly out of their way, and do npm install https://registry.npmjs.org/foo/-/staged/some/kind/of/uuid/foo-1.2.3.tgz, well, people will just do that. Let's not make people jump through dumb hoops. It's the equivalent of disabling copy/paste on a password field.

Requiring a login to install a staged public package is also not reasonable, and bloats the complexity for implementing this pretty significantly.

I'm not ready to throw the baby out with the bathwater here. Let's not steer so far away from a hypothetical DX problem without at least thinking through how to fix it, and establishing how much of a problem it really is, and providing gentler guardrails to help people avoid it.

Here's what I propose:

  • When installing, if --include-staged is not provided, then staged versions are never considered. (This solves like 90% of the DX issue, because you have to opt in to using a staged package in the first place.)
  • If --include-staged is provided, then staged packages are still lower priority than published packages. (This solves 90% of the rest; the chances of getting a staged package by accident are effectively nil, and a published version will always be used if possible.)
  • When promoting, ensure that the dist.tarball path of the staged copy of the tarball continues to be valid and resolve to the final published URL of the tarball (possibly via a 301 redirect). (This solves most of what's left; if you lock a staged package that then gets promoted, nothing breaks.)
  • When overwriting a staged version, the tarball url changes, and the previous staged tarball URL responds with ideally a 410 Gone, but may respond with a 404 Not Found. (And here we have our tiny fraction of cases which will actually affect DX in a negative way. But it's clear, you opted in, the error message says what to do, and you probably want to take note of this anyway.) A redirect is not appropriate in this case, because the integrity will be different, so anyone fetching from the previous location is going to get a harder to debug integrity error if we redirect it.

This is a lot simpler, lets the registry be in charge of what's what, and lets the CLI just do its current default behavior with the slight addition of checking packument.stagedVersions.versions for matches. (Ie, just a change to npm-pick-manifest, but no changes required to any other part of the system, at least for installation.)

The authorization is still just what it always is. No special auth for special parts of the packument. If you can publish, you can publish, if you can see it, you can install it.

The resulting data structure could look something like this:

{
  "name": "the-world",
  "dist-tags": {
    "latest": "1.2.3"
  },
  "versions": {
    "1.2.3": {
      "name": "the-world",
      "description:": "is a stage",
      "version": "1.2.3",
      "_npmUser": "publishing-stager",
      "_npmUserPromoted": "promoting-stager",
      "dist": {
        "tarball": "https://registry.npmjs.org/the-world/-/the-world-1.2.3.tgz",
        "integrity": "sha512-deadbeefcafebadholymolywowsuchstage"
      }
    }
  },
  "stagedVersions": {
    "log": [
      "maybe some kind of bookkeeping here?  what versions staged at which time, etc."
    ],
    "versions": {
      "1.2.4": {
        "dist": {
          "tarball": "https://registry.npmjs.org/the-world/-/staged/sha512-content-hash-url-safe-base64/the-world-1.2.4.tgz",
          "integrity": "sha512-content-hash-normal-base64"
        }
      }
    }
  }
}

The registry could then have a 301 redirect set up from any /:pkg/-/staged/:content-address/:file to the appropriate file. This wouldn't even have to be done explicitly for staged promotions; any file could be fetched this way, and it could be kept up to date with each file we store. And, since the url is deterministically different from the final artifact url, we can have different caching semantics around it. (Or not - the content-address is right in there, it's not like anything else could ever be at that URL.)

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2020

If we wanted to go the extra mile for the user, Pacote could detect these 404 errors for tarballs, see that it's a staged tarball URL, re-fetch the packument and update the resolved value.

I start to worry a bit that that might be too magical, but otoh, maybe it's exactly the right amount of magical. In any event, we could explore that with this data and URL structure, and decide later whether it's worth doing.

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2020

The net net on this would be:

  • Use a staged package: npm install foo@2 --include-staged (as long as 2.x versions are all in staging, like if it's a beta or somethign, and 1.x is the published versions).
  • When foo@2 is promoted, then npm install foo@2 --include-staged will pick up the promoted version.
  • If the staged foo@2 depends on a staged bar@3, then ok, that'll Just Work, because you're in "include staged" mode.
  • When the pkg is promoted and the resolved changes, the package lock doesn't break. (It might get slightly less canonical or cacheable, but then again, it might not. Maybe we want to have content-addresses in our tarball urls!)
  • If the staged version is overwritten with a new staged version, then the build will break (unless Pacote is taught to just handle this, which may be worthwhile, and would be possible.)

I don't see any DX issues that justify restricting the utility of staged packages, but maybe I'm missing something.

@ljharb
Copy link
Contributor

ljharb commented Feb 17, 2020

To me, the entire point of the feature is to be able to avoid committing to anything published - and therefore usable - prior to promotion. I want to be able to, for example, have every master build in CI generate a staged version, but have them all secret - ie, impossible to even view for non-owners - until an owner selects one to promote.

I would prefer to see "testing a staged version" as a non-goal, since that's what prereleases are for - the value here for me is "i don't want to let anyone but a human do npm publish, but i want to be able to do all the prep steps for publishing separately, and without 2FA" (which i believe is also the use case for conventional-release?)

If 2FA is not required to stage something, and staged version can be installed, then that seems like a pretty large potential security hole for anybody using --include-staged?

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2020

In a case where you have a lot of packages that all work together, I'd really want to have 2 integration tests run before any publish is promoted from staging to release:

  1. Run this module's tests against all published versions of the others in the set.
  2. Run this module's tests against all staged versions of others in the set.

Otherwise, it's not really an integration test, because you're not testing the integration with current and next.

As for using prereleases for this, yeah, you can effectively get all of this with prereleases. But part of the goal of this feature is that it lets you minimize version churn (including prerelease version churn in git history). This allows for something in between "npm install github:user/foo#next" and "npm install foo@next". Of course there's already ways to do all of this already; there's ways to do everything already (including publishing from CI while still having TFA protection). They're just less convenient. I don't see why making staged publishes less convenient is a value. What harm comes from it?

If we make staged publishes effectively hidden, then we also make it intolerably friction-ful to have the registry decide to temporarily stage publishes that appear to be suspect. If the staged copy is available behind a flag, then the user is temporarily inconvenienced, but not completely blocked in that scenario.

If 2FA is not required to stage something, and staged version can be installed, then that seems like a pretty large potential security hole for anybody using --include-staged?

I don't think so. (a) It's opt-in. So, yes, you can shoot yourself in the foot by setting it in a .npmrc config or something, but you can also get into very similar trouble installing from a git repo. And (b), staged package versions would always be deprioritized, so even when opting in, you'd only get the staged copies if they're the only thing that satisfies the dep. (Allowing a monorepo to publish 100 packages to staging, verify that they all work together, fix whatever problems occur, and then promote them all together.)

@isaacs
Copy link
Contributor

isaacs commented Feb 17, 2020

Also, 2FA is not required to publish today, so this isn't really much of a reduction in the security stance. Requiring 2FA for all publishes would be a big step, and I'd get ready for torches and pitchforks as a result.

@ljharb
Copy link
Contributor

ljharb commented Feb 17, 2020

To clarify; I believe the desire (which i share) is for a package owner to require 2FA for promotion but not for staging, precisely so that it's convenient.

I guess I don't understand why "less version churn" is a value - https://twitter.com/izs/status/603936197941993476

@dominykas
Copy link

Just to clarify:

  1. Someone installs a package with --include-staged
  2. It gets added to a lock file
  3. Someone else runs npm install for that lock file (the version is still staged)

Do they get an error (because the package is staged) or does everything proceed as if it's business as usual (because it's in the lock file and integrity matches)? Or do they get a confirmation step or at least a warning?

@isaacs
Copy link
Contributor

isaacs commented Feb 18, 2020

@dominykas With the approach I'm proposing, the tarball url would get in the lockfile (default behavior today, in other words). The subsequent npm install user would get that staged version (if it's still available and has not been overridden), because fetching the tarball would be allowed. (npm does not fetch the packument unnecessarily if it can get everything it needs from the lockfile, so it has no way of knowing that it's a staged version; it's just a pre-resolved tarball url.)

This is required to be able to run headless CI jobs against staged versions of things, which is a use case I'd like to be able to support.

@isaacs
Copy link
Contributor

isaacs commented Feb 18, 2020

I guess I don't understand why "less version churn" is a value - https://twitter.com/izs/status/603936197941993476

Yes, as we all know, there's no shortage of integers. But one of the use cases that has been brought up in reference to this feature is being able to more deterministically test and release projects that are composed of many different packages. Prerelease tags aren't an ideal fit for that use case.

nateghia219

This comment was marked as spam.

@dominykas
Copy link

Is there any chance this is going to happen? Should we just close it to avoid having it as a bot spamhole?

@kelset
Copy link

kelset commented Oct 28, 2022

cc @darcyclarke 👆 so many bots spamming in this RFC 😥

Xcalibur53

This comment was marked as spam.

113ll3

This comment was marked as spam.

113ll3

This comment was marked as spam.

@Abdulqadirasif
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion is pending a discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet