doc: initial flow #1

BethGriggs · 2020-05-14T14:53:41Z

Before getting started writing the tool, we should define the expected flow of operation. @andrewhughes101 and I discussed this and tried to write down the end-to-end flow.

I have PR'd this to the README so that we can get line-by-line comments. All file names and formats are up for discussion.

Open questions from our discussion

Should the PR would be opened in the person running its name?
- Would the tool therefore need to have access to generate a GitHub token?
Do we want the draft PRs to be merged?
- Assuming not, so opens only a draft PR.
- Would maintainers be upset that many draft PRs get opened and never merged?
Do we want this tool to execute on creation of a specific branch name pattern rather than manually kicking off runs locally?
- If so, how and where would the wiby results be presented?

ping @dominykas @wesleytodd

ljharb · 2020-05-14T15:03:26Z

README.md

+
+1. Define the list of dependents you want to test against in a `wiby.json`.
+1. Push the release candidate branch or tag to GitHub.
+1. Indicate that you want to start your dependent tests by running `wiby test` locally while on your release candidate branch.


the list of dependents seems like it should be in the local working copy too?

ljharb · 2020-05-14T15:05:53Z

README.md

+1. Indicate that you want to start your dependent tests by running `wiby test` locally while on your release candidate branch.
+1. `wiby` will look for your defined list of dependents to test against in `wiby.json`.
+1. For each module in the `wiby.json`, retrieve their `package.json` and determine whether the module is likely to automatically be updated to the new version of your module once it is released.
+    - For example, if you're creating a release candidate for `v2.1.0-rc`, and the dependent module is using the range `^1.x.x`, then we should not open a test PR.


There’s really two situations you want to test tho - one is “I’m trying not to break you with this non major bump” and the other is “I’m making a semver-major but i want to know who it breaks and how”

Ah, we did not have major releases in mind. Maybe there should be a --major option? Or --force that would not do the 'Will you be automatically updated to this version?' check. But maybe that should be the default 🤔

The motivation behind this was to try and minimize the influx of draft PRs (especially if the dependent is unlikely to pickup the new release candidate version).

Another approach could be to allow maintainers to be able to opt-in to either/both applicable range and major updates in the wiby.json(final name/format TBD). Then external dependent maintainers can PR their module and indicate whether they want to be tested against majors in the wiby.json file.

I think my idea below might help. If the tool forks the repo to your personal account before opening the PR it will reduce the noise. This also allows you to be more flexible for when you run it, because I imagine that I would want to run this before ever pushing, especially if it was just an idea to test out.

README.md

wesleytodd · 2020-05-14T21:43:59Z

README.md

+# Flow
+
+1. Define the list of dependents you want to test against in a `wiby.json`.
+1. Push the release candidate branch or tag to GitHub.


I don't know if this is intentionally excluding other workflows but it seems to me that some project might see this more as a nightly job. We should just make sure the solution will run well as an action on commit or a cron.

wesleytodd · 2020-05-14T21:47:52Z

README.md

+1. Push the release candidate branch or tag to GitHub.
+1. Indicate that you want to start your dependent tests by running `wiby test` locally while on your release candidate branch.
+1. `wiby` will look for your defined list of dependents to test against in `wiby.json`.
+1. For each module in the `wiby.json`, retrieve their `package.json` and determine whether the module is likely to automatically be updated to the new version of your module once it is released.


This is a great addition! I do think there are cases where you want to test even if it is not auto updated. For example, express locks many of it's deps. I imagine ensuring you dont break express when it is time for them to updates will be important.

wesleytodd · 2020-05-14T21:50:20Z

README.md

+1. `wiby` will look for your defined list of dependents to test against in `wiby.json`.
+1. For each module in the `wiby.json`, retrieve their `package.json` and determine whether the module is likely to automatically be updated to the new version of your module once it is released.
+    - For example, if you're creating a release candidate for `v2.1.0-rc`, and the dependent module is using the range `^1.x.x`, then we should not open a test PR.
+1. Patch the `package.json` of the dependent module to point to the release candidate branch for your module.


This will need to be a bit more complicated than just that. It should probably use something like https://www.npmjs.com/package/preferred-pm to detect which package manager then run npm update or yarn upgrade, etc.

wesleytodd · 2020-05-14T21:51:36Z

README.md

+1. For each module in the `wiby.json`, retrieve their `package.json` and determine whether the module is likely to automatically be updated to the new version of your module once it is released.
+    - For example, if you're creating a release candidate for `v2.1.0-rc`, and the dependent module is using the range `^1.x.x`, then we should not open a test PR.
+1. Patch the `package.json` of the dependent module to point to the release candidate branch for your module.
+1. Open a draft PR in the dependents repository that contains the patched `package.json`.


I think it should fork the repo and open a PR there. If you do it against the actual repo it clutters it up, and also might result in notifications to maintainers.

Yeah using forks for testing makes sense to me to avoid noise. My concern would be whether it is easy to automate the whole flow using forks? I'm sure the 'fork repository' part would be reasonable to implement. But, would it involve enabling some CI related WebHooks or settings on the forked repository? What if the module is using credentials in their Travis or Actions set up, would we need to manually define those?

Another thought - if there are CI allowances (minutes, runs), using forks moves the cost to the person or organisation running the tool. I think I agree that the person running the tool should bear the resource cost of the testing.

Indeed; the newly created fork would need to be hooked up to CI. that's why greenkeeper et al all use branches on the source repo.

You're also absolutely right that this means the maintainer bears the cost / threshold usage; if there were an easy way for the initiator to bear it that'd be ideal.

(just documenting roughly what I said during the last meeting)

I think the tool that runs the tests should mostly not care whether it's opening the PRs against a fork or against the original repo. It should have an option to update the repo's master if it's a fork, though. That's probably the first thing to check/do in the flow.

I think it's useful to also have a subcommand in the tool which allows creating the forks - the subcommand should be smart enough to call the relevant APIs to enable Travis/Github Actions (whatever else it wants to support) on the fork. But I don't think that's necessary to get started with this.

I think that the etiquette should be that you either have an explicit permission from the original author to open the PRs in their repo or you have to use a fork. I think larger organizations could have a full separate organization, with just the forks, used purely for testing to reduce noise. That could be a separate tool altogether (which is I wouldn't rush to build the forking support into wiby).

Does GitHub disable actions on forks by default? I know for travis that is a tough problem. So I do see the problem here, but if I am consistently getting opened PRs on a popular project from this tool across all my dependencies, I am going to get quite annoyed with them. Distributing that across the entire ecosystem will be near on impossible for everyone to get permission and not annoy people. This I think will be the biggest sticking point for this project.

The config for the jobs is there, so there must be an API to enable these jobs? Forking and releasing from a fork is a fact of life, so testing must be possible?

dominykas · 2020-05-23T09:26:36Z

README.md

@@ -17,4 +17,19 @@
 1. Open a draft PR in the dependents repository that contains the patched `package.json`.
 1. `wiby` will create a JSON file (`wiby-v2.1.0.json`) which contains the links of dependent PRs that have been opened.


Why do we need the version in the filename?

Ah, looking back this is unclear. I was thinking that the commit/branch/tag that we run against should be injected into the filename. But, that information could be stored within the file itself.

dominykas · 2020-05-23T09:31:35Z

README.md

+1. For each module in the `wiby.json`, retrieve their `package.json` and determine whether the module is likely to automatically be updated to the new version of your module once it is released.
+    - For example, if you're creating a release candidate for `v2.1.0-rc`, and the dependent module is using the range `^1.x.x`, then we should not open a test PR.
+1. Patch the `package.json` of the dependent module to point to the release candidate branch for your module.
+1. Open a draft PR in the dependents repository that contains the patched `package.json`.


(just documenting roughly what I said during the last meeting)

I think the tool that runs the tests should mostly not care whether it's opening the PRs against a fork or against the original repo. It should have an option to update the repo's master if it's a fork, though. That's probably the first thing to check/do in the flow.

I think it's useful to also have a subcommand in the tool which allows creating the forks - the subcommand should be smart enough to call the relevant APIs to enable Travis/Github Actions (whatever else it wants to support) on the fork. But I don't think that's necessary to get started with this.

I think that the etiquette should be that you either have an explicit permission from the original author to open the PRs in their repo or you have to use a fork. I think larger organizations could have a full separate organization, with just the forks, used purely for testing to reduce noise. That could be a separate tool altogether (which is I wouldn't rush to build the forking support into wiby).

dominykas · 2020-05-23T09:35:36Z

README.md

+```json
+{
+    "express": "all",


Would it make sense to structure this as an array of objects, so that it's easier to extend in the future? e.g.

{ "dependents": [ { "name": "express", "repo": "git+https://....", "triggers": ["all"], "setting1": "value1" }, { "name": "express-session", "repo": "git+https://....", "triggers": ["rc"], "setting2": "value2" } ], "config": { "global-setting": "value3" } }

Maybe triggers is not even necessary for the initial release? If this starts out as a CLI tool, then it's always an explicit call? e.g. $ cd my-repo && git checkout my-new-feature && wiby test and then after some time $ wiby results, which would loop through the PRs it can find and use the status API to report back?

Having a CLI tool means that it's easy enough to build a bot or a GH action or even Jenkins job (for those of us who are stuck in the past decades...), really. Both commands should be able to return results as json so that this can be used as a library too.

It should probably also include some fields from https://github.com/nodejs/citgm/blob/master/lib/lookup.json?

dominykas · 2020-05-23T09:58:12Z

Here's a few other things to consider:

What if you want to test another-repo in your dependents as well? e.g. you're updating express and cookie and you want to test if the new combo of the two works well with express-session.
- In the past, I've solved similar problems by using the same branch name as a convention. E.g. both express and cookie have a my-feature branch. You run wiby in express, it creates a wiby/my-feature branch in express-session and opens a draft PR tehre. When you run wiby again in cookie, it can see that wiby/my-feature already exists on express-session, and instead updates the existing PR, rather than opens a new one
- Juggling branch names might be a bit too much (the namespace is a lot larger on public GH), so maybe just building something like wiby --test-id=my-feature to generate an identifier when you want to test the same thing, but otherwise use unique ID for each call? Record that on the status check to be able to retrieve and correlate results?
When wiby sees that there is already a PR in the dependent with the same branch (or ID, if using IDs)
- Solving similar problems in the past, I've observed that people start off their testing process, and it takes a while. In the meantime, the dependent moves forward - and their tests are now invalid and out of date.
- This means that wiby should probably have an option to rebase or reapply the changes it makes.
- Note that in the case of failing tests, the author of the dependency might want to make changes in the dependent and then re-run the tests again, so a rebase might not always be easy, but wiby should at least be able to report that somehow.
There should be a param for what to use a base branch (so that you can test how your new release affects older versions that are still supported via LTS or otherwise).
Just a gotcha to keep in mind - when updating package.json to install stuff from git, the deduplication might not work if the dependency is using semantic-release, in which case it usually has a 0.0.0-development as the version in its package.json. Can't wait for npm overrides :)
We've briefly touched on an issue of various access tokens to execute tests in the dependent. There is a generalized problem of needing to tweak the test scripts/environment in general, when using the fork (or even otherwise). So probably wiby should have a way to execute some more code and make some more changes when it is preparing to open a PR. This is probably very much unnecessary for the initial version, but maybe it's worth keeping in mind, to make sure we leave enough extension points for the future.

dominykas · 2020-05-23T10:09:29Z

Should the PR would be opened in the person running its name?
Would the tool therefore need to have access to generate a GitHub token?

Depends on how the tool is used, I would say. If we're starting with the CLI, it may as well just rely on the presence of GH_TOKEN in the env (as much as I hate keeping one my local dev env). There's probably libraries out there which provide a way to acquire the GH_TOKEN from various sources (Keychain Access, git credentials helper, ~/.hub/config, etc). If there aren't - well there's a chance to create one ;)

Not sure the tool needs to generate any tokens, though?

Do we want the draft PRs to be merged?
Assuming not, so opens only a draft PR.

Yeah, I would say the PRs are unlikely to get merged, because they modify the package.json to install from a git branch - this is not something you'd want to publish into npm. However these PRs may sometimes require further changes to make the tests pass in the dependent - and these changes may need to get merged before/after the new version of the dependency is released, so wiby should probably be smart about not closing such PRs.

Would maintainers be upset that many draft PRs get opened and never merged?

I would guess so. I think wiby could auto-close the PRs, as long as they have no modifications, after the original PR is closed/merged. This is similar to how renovate works too - if you modify a renovate PR, it posts a comment and says it will leave it alone. TBH, I think there's quite some good practices - implementation and behavior - to be learnt from renovate when building this.

Do we want this tool to execute on creation of a specific branch name pattern rather than manually kicking off runs locally?

I think auto-execution might be too much and would generate way too much noise. Think about how people are annoyed about dependabot - which opens PRs for real releases. And wiby would do that for things that aren't even released yet.

If so, how and where would the wiby results be presented?

In the CLI initially, and in a PR comment going forward, similar to how coveralls does it? wiby could be smart enough to leave a marker inside the comment and keep updating that comment with its own metadata and latest results.

dominykas · 2020-05-23T11:01:30Z

Just occurred to me that this doesn't need a wiby.json for the very very simplest case - all it needs is a wiby test <repo url> command support.

BethGriggs · 2020-05-28T17:28:51Z

So i've tried to update the flow slightly, but it's not complete! In parallel, we're currently (@andrewhughes101 and I) working on implementing the first prototype to handle the simplest cases - pretty much wiby test <repo url>.

Thanks for the input @dominykas (and others). I have read all the comments but haven't had time to think enough about some of them.

What if you want to test another-repo in your dependents as well? e.g. you're updating express and cookie and you want to test if the new combo of the two works well with express-session.

Need to think about the flow of this case a bit more.

There should be a param for what to use a base branch (so that you can test how your new release affects older versions that are still supported via LTS or otherwise).

Base branch of the dependent? I think we can add this to your suggested wiby.json structure. But, for the wiby random type cases it wouldn't be possible to determine so we'd default to master.

BethGriggs · 2020-07-06T12:45:57Z

Does it make sense to land this as-is and iterate? I don't think it'll be possible to capture the agreed end-to-end flow in just one PR. I think it'd be nice to have a descriptive README sooner, and we can continue to edit it as the tool evolves?

dominykas · 2020-07-06T13:46:01Z

I'm very much in favor of landing this info, but I'm not sure we should land it inside the README. I feel that the README should reflect the current state of the tool - so maybe we should move this into a docs/rfc folder?

BethGriggs · 2020-07-06T17:01:06Z

I don't love the idea of having a docs/rfc in the repository - do we want to encourage future rfc's to be PR'd into the repository? Perhaps even naming it DRAFT-FLOW.md or something at the top-level would work?

I'm happy to move it to another file either way - and if you think docs/rfc is the best way to go i'll update the PR to do so.

dominykas · 2020-07-06T18:40:21Z

I'm not set on that idea - just figured it might be a path forward. We can always split that out into a separate repo when we need that?

But a separate file works too, I suppose.

BethGriggs · 2020-07-06T20:16:46Z

I've just moved it to docs/DRAFT-FLOW.md - does that work for now?

mhdawson

LGTM, since it tagged as Draft flow seems reasonable, and we can adjust as we get move experience as the tool is built.

mhdawson · 2020-07-21T13:31:10Z

@BethGriggs can you resolve the conflicts ?

BethGriggs · 2020-07-21T13:40:18Z

@mhdawson, resolved

wesleytodd

My comments on the fork stuff still stand, but I think we can iterate on this as we start using it and see how it goes. Sorry for being a delay here!

dominykas · 2021-01-03T10:09:37Z

🎉 This PR is included in version 0.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

doc: initial flow

5d19bfd

ljharb reviewed May 14, 2020

View reviewed changes

wesleytodd requested changes May 14, 2020

View reviewed changes

BethGriggs added 2 commits May 20, 2020 14:53

doc: capture further details

07d2a2c

doc: draft wiby.json structure

71a5933

dominykas reviewed May 23, 2020

View reviewed changes

doc: update flow based on review comments

6fff904

wesleytodd mentioned this pull request May 28, 2020

Discuss modules in CITGM nodejs/web-server-frameworks#35

Open

BethGriggs requested a review from wesleytodd June 29, 2020 17:36

docs: move flow to docs/DRAFT-FLOW.md

3834cc7

dominykas approved these changes Jul 6, 2020

View reviewed changes

andrewhughes101 approved these changes Jul 7, 2020

View reviewed changes

mhdawson approved these changes Jul 21, 2020

View reviewed changes

BethGriggs and others added 2 commits July 21, 2020 14:36

Merge branch 'master' into initial-flow

b278fd0

fixup! blank line

e0b8ec3

wesleytodd approved these changes Jul 23, 2020

View reviewed changes

BethGriggs merged commit 342e552 into pkgjs:master Jul 23, 2020

dominykas added the released label Jan 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: initial flow #1

doc: initial flow #1

BethGriggs commented May 14, 2020

ljharb May 14, 2020

ljharb May 14, 2020

BethGriggs May 14, 2020

wesleytodd May 14, 2020

wesleytodd May 14, 2020

wesleytodd May 14, 2020

wesleytodd May 14, 2020

wesleytodd May 14, 2020

BethGriggs May 15, 2020

ljharb May 15, 2020

dominykas May 23, 2020

wesleytodd Jun 30, 2020

dominykas Jun 30, 2020

dominykas May 23, 2020

BethGriggs May 23, 2020

dominykas May 23, 2020

dominykas May 23, 2020 •

edited

Loading

dominykas commented May 23, 2020

dominykas commented May 23, 2020

dominykas commented May 23, 2020

BethGriggs commented May 28, 2020

BethGriggs commented Jul 6, 2020

dominykas commented Jul 6, 2020

BethGriggs commented Jul 6, 2020

dominykas commented Jul 6, 2020

BethGriggs commented Jul 6, 2020

mhdawson left a comment

mhdawson commented Jul 21, 2020

BethGriggs commented Jul 21, 2020

wesleytodd left a comment

dominykas commented Jan 3, 2021

		@@ -17,4 +17,19 @@
		1. Open a draft PR in the dependents repository that contains the patched `package.json`.
		1. `wiby` will create a JSON file (`wiby-v2.1.0.json`) which contains the links of dependent PRs that have been opened.

doc: initial flow #1

doc: initial flow #1

Conversation

BethGriggs commented May 14, 2020

Open questions from our discussion

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominykas May 23, 2020 • edited Loading

Choose a reason for hiding this comment

dominykas commented May 23, 2020

dominykas commented May 23, 2020

dominykas commented May 23, 2020

BethGriggs commented May 28, 2020

BethGriggs commented Jul 6, 2020

dominykas commented Jul 6, 2020

BethGriggs commented Jul 6, 2020

dominykas commented Jul 6, 2020

BethGriggs commented Jul 6, 2020

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Jul 21, 2020

BethGriggs commented Jul 21, 2020

wesleytodd left a comment

Choose a reason for hiding this comment

dominykas commented Jan 3, 2021

dominykas May 23, 2020 •

edited

Loading