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

Review/release process #3

Open
rubensworks opened this issue Oct 26, 2020 · 14 comments
Open

Review/release process #3

rubensworks opened this issue Oct 26, 2020 · 14 comments

Comments

@rubensworks
Copy link
Member

Let's discuss the review and release process that we want to follow in this package.

I propose the following:

  • Creation of a GH team 'types' that will always be asked to review for each incoming PR.
  • Merging can happen as soon as one of the following happens:
    • All members of the 'types' team approved the PR.
    • At least one member of the 'types' team has approved the PR, and a week has passed.
  • As soon as merging is done, a release is made (exception is when multiple breaking changes will aggregated for a release)

I would give everyone of the GH team 'types' npm publish rights. The person who merges the PR is the one who publishes to npm.

@tpluscode
Copy link
Contributor

tpluscode commented Oct 26, 2020

I would give everyone of the GH team 'types' npm publish rights. The person who merges the PR is the one who publishes to npm.

Just make this automatic and publish from GitHub Actions on new tag

@blake-regalia
Copy link
Contributor

At least one member of the 'types' team has approved the PR, and a week has passed.

Let's make it two weeks to give people some time.

@rubensworks
Copy link
Member Author

Let's make it two weeks to give people some time.

Two weeks can be a very long time in some cases though. E.g. when devs discover a bug in the typings, and may be required to wait two weeks before a fix is merged.

@bergos
Copy link
Member

bergos commented Oct 27, 2020

Two weeks is what we agreed on for the specs, but I see that for code it could be a long time. If it doesn't make the process too complicated, maybe one week for patches, two weeks for changes that will result in a new minor/major version?

@blake-regalia
Copy link
Contributor

blake-regalia commented Oct 27, 2020

Yeah, some compromise may be needed. Just need to avoid PRs that only get reviewed by one or two people while others are unavailable. What if we did something like this:

  • PR has been reviewed and approved by all 'type' members
  • PR has been reviewed and approved by at least 3 'type' members (we can make it 2 for PRs opened by core team) and a week has passed
  • PR has been reviewed and approved by at least 1 'type' member and 2 weeks have passed

We can adjust those numbers based on consensus.

**Edit: ** I might actually prefer @bergos suggestion or some hybrid. Want to keep it simple but always hard to anticipate what protocols will be like in practice.

@rubensworks
Copy link
Member Author

Both suggestions sound good to me, no preference for either one on my end.

This is also something we can tweak afterwards if we notice that things are not working out.

@alexkreidler
Copy link

I'd also propose we

  • keep a changelog up to date
  • set some guidelines on versioning (since this repo contains the types for 3 different specs, it'd be somewhat difficult if they were versioned separately.) See also WebIDL sequence as function/method paramater dataset-spec#6
  • publish some documentation on the types (most likely just generated from the code comments)

There are a few tools to do things like this: changesets, api-extractor, typedoc

@tpluscode
Copy link
Contributor

👍 to atlassian changesets but they will make deployment more difficult to automate. I have never really tried their proposed GitHub Actions workflow

since this repo contains the types for 3 different specs, it'd be somewhat difficult if they were versioned separately.

And what if the 3 types packages were indeed 3 types packages.

  • @rdfjs/spec-data-model
  • @rdfjs/spec-dataset
  • @rdfjs/spec-stream

And then we can keep rdf-js to simply re-export for those above. Setting it up like this makes all the more sense to adopt changesets

@alexkreidler
Copy link

That's an interesting thought, but I think since the types are fairly coupled together: dataset.d.ts depends on data-model.d.ts and stream.d.ts, stream.d.ts depends on data-model.d.ts, versioning might be more complicated (e.g. does the @rdfjs/spec-stream have @rdfjs/spec-data-model as a dependency or a peerDependency? (because @rdfjs/spec-dataset would have it as a dependency)) The typescript team recommends that users of types-only repos (like from DT) pin their package.json versions exactly, because even a minor version update changes types. I think if we were to split them up it might be valuable to version them in lock-step.

I don't see a real reason to split them up now, but if @rdfjs wanted to move the other packages it publishes into a monorepo, and we had things like a DataFactory implementation, maybe it would make more sense to do so.

@rubensworks
Copy link
Member Author

I also don't think splitting is needed. A single typings package should be fine.

Regarding changelogs, I don't have a big preference, as long as it doesn't put too much overhead on the release process.
For reference, I use my own custom changelog generator in my tools, which requires only minimal effort.

@tpluscode
Copy link
Contributor

Splitting would be useful if we'd want to version the types for each spec independently. Dependencies between them are not a problem and changesets handles that nicely. Otherwise the preferred usage will be to install the meta package anyway.

Otherwise, I've been using standard-version for generating version increments and changelog based on conventional commit messages. That has a downside thought that requires rewriting history is we make mistakes and is a potential barrier for contributors and a burden on maintainers.

Yes, thinking out loud here, I would vote for changesets regardless of the single vs multiple packages

@tpluscode
Copy link
Contributor

Let's push this forward. We are almost done with the publishing. Might get back to the organisational decisions.

How do you reflect on your original proposals @blake-regalia? I see that entire team core has admin right to the repo. DO you intend to keep it that way?

@blake-regalia
Copy link
Contributor

I created a types team and started adding people; we should follow this thread up with a PR for adding a PR template and contributing guidelines in README. I can get this started

@tpluscode
Copy link
Contributor

About teams and access, I would also reduce core team to Triage because technically anyone with merge permission can release a new version.

Also, I propose branch protection rule for master to require reviews.

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

No branches or pull requests

5 participants