search

pkgjs / wiby

"Will I break you" - a tool for testing dependents
Apache License 2.0
33 stars 7 forks source link

Open pull-requests when testing #53

Closed dominykas closed 3 years ago

dominykas commented 3 years ago

Ref: https://github.com/pkgjs/wiby/blob/master/docs/DRAFT-FLOW.md step 9

Open a draft PR in the dependents repository that contains the patched package.json.

Depending on the CI setup, it might not be enough to just push the branch - wiby.json should allow setting a configuration option to additionally open a PR.

This also means that we'd probably need to close the PR, etc, when the tests are done.

dominykas commented 3 years ago

@ghinks and maybe this one?

ghinks commented 3 years ago

ok, will look at this one over the next few days.

dominykas commented 3 years ago

Let me know if you want to discuss it beforehand!

ghinks commented 3 years ago

I think we should discuss it. Let me re-read and respond today. Been super busy.

ghinks commented 3 years ago

Had a quick look, implementation looks straightforwards enough. However there will always be questions.

Options

There are some gotchas.

I'll put something together in draft.

ghinks commented 3 years ago

@dominykas @rodion-arr I was wondering if it would be a good idea to split this into 2 parts.

your thoughts?

I'm trying to avoid bigger PRs.

rodion-arr commented 3 years ago

It looks like pretty much separate features, imo. I'm not against separate PRs but maybe it's better to release them together (if we have any users at all at this stage)

ghinks commented 3 years ago

yes agreed, release together. But I shall raise 2 PRs.

dominykas commented 3 years ago

CLI Option Name. Should we have a command line option like pull-request?

Yeah, CLI convention is for words to be separated by - in the args, and Yargs will automatically camelCase that when using in the code.

Wiby config option. Should we have an option in the configuration pull-request: boolean?

I suppose since Yargs transforms that into camelCase, the wiby.json should also use pullRequest?

Just because we have a github token it does not mean it can create a PR. Do we want to check , not sure we can yet but will look into it

I'm not sure there's a need to check this, but the GH token will have to have push access (for now) to be able to create the branch - and I'm pretty positive if you have push access, you have PR access? Even in the future, we'll need to have push access into a fork even if we want to PR into upstream, so we'll fail way before this if the token doesn't have correct permissions.

Should we check that the PR has not already been raised? I think when I played with the API it would not let me raise the same PR twice even with different titles

That's a good question.

At the moment, it's not a problem, because we still don't have support for re-using existing branches (bullet point in #78), so we'd fail before the check anyways.

Going forward, though, I think we'll need that check, yes - you can only have a single PR from the same target to the same base (i.e. our test branch to default branch), so it would probably fail if the PR already exists.

the first raising the draft PR the second checking and closing the PR if the tests passed

Yeah, I think it's fine to do it as separate PRs, whichever you feel like.

Just to clarify - there's no need to do any specific status checks (if that's what you meant), because status checks belong on commits.

We will need to update wiby clean to close the PRs (if any) before deleting the branches. I wonder if getExistingRepoBranches can return the PRs associated with a branch 🤔

dominykas commented 3 years ago

Added in #93 - will release once #100 is merged and master is green again.