Feature/close draft pr #93

ghinks · 2021-04-10T11:12:24Z

Changes for issue 53

Both opens the draft PR on test and closes it on result

using --pullrequest, open to use of other names

goes in ontop of pull 91 tap update

raise a PR when the --pullrequest=true or if set in the wiby.json file.

add a body and title to the PR

remove the PR during the result scan

add tests for PR closing

change the status and the conclussion to values defined in [checkrun definitio](https://docs.github.com/en/[email protected]/graphql/reference/objects#checkrun)

.wiby.json

lib/github.js

dominykas · 2021-04-11T09:55:10Z

lib/result.js

@@ -37,6 +37,7 @@ module.exports = async function ({ dependents }) {
    const parentBranchName = await context.getParentBranchName()
    const branch = await context.getTestingBranchName(parentBranchName)
    let resp = await github.getChecks(dependentPkgInfo.owner, dependentPkgInfo.name, branch)
+    await updatePRStatus(dependentPkgInfo.owner, dependentPkgInfo.name, branch, resp.data.check_runs)


🤔

So we do have a wiby clean command, which deletes the branches. So maybe that's a better fit for this?

I'm kind of torn on whether the full wiby clean should happen automatically after the results are successful (it seems it now does it regardless of the result?) - I think if, as an implementer of a feature, I run wiby result, and there's a failure - I definitely want to keep things as is, so that I can go explore and debug. But if everything passed - I might still want to keep things around, if I'm building a feature that spans multiple repos?

Quite possibly this is a full-on separate feature and/or needs to be behind a global config flag (--auto-clean / autoClean in wiby.json or smth).

agreed.
Let me address the other issues in this draft PR. Then we can discuss further and maybe move it behind a different command.

dominykas · 2021-04-11T09:58:32Z

👏 Thanks. sorry for not responding and not clarifying things earlier... I see there's some other discussions on the issue - will check those out now 👍

change updatePRStatus to closePR

change pullrequest to pull-request change pullRequest (json) to pullRequest

change the pullRequest to be per dependent object

lib/test.js

lib/result.js

dominykas · 2021-04-18T08:18:43Z

lib/result.js

+  }
+  const prsToClose = checkRuns.reduce((acc, check) => {
+    if (check.status === 'completed' &&
+        check.conclusion === 'success' &&


I forgot... do we not already have a helper function to verify if check has passed?

not sure I see this else where?

Yeah, OK, the one we have is in a different context 👍

dominykas · 2021-04-18T08:22:54Z

lib/test.js

+  `
+  const head = context.getTestingBranchName(parentBranchName)
+  const draft = true
+  const result = await github.createPR(dependentOwner, dependentRepo, title, head, parentBranchName, draft, body)


Do we need to set the base to parentBranchName here? I do have a note in the issues, that if the branch exists in the dependent repo as well as in parent repo, we should try to use that, but that's not implemented yet, so at this point parentBranchName likely does not even exist - we should probably keep it undefined (if the API allows it - otherwise we need to fetch the default branch name...)?

I was not sure on this comment, but its late afternoon so I'm taking it easy.

@dominykas ok, I came back to look at this comment and confess to not understanding at first you are referring to this I think issue 95. If so I think I understand.

It's kind of related to that issue, but that's not exactly it.

When opening a PR, we want to merge head into base. So most of the time base should be main / master (i.e. default branch), not parentBranchName?

Suppose you're working on my-feature

You wiby test

A wiby-my-feature branch is created in the dependent repo

You should be opening a PR on the dependent repo to merge wiby-my-feature into main

i.e. my-feature (parentBranchName) will not even exist in the dependent, most of the time. When it does - we do have the "existing branches" case, which needs to be thought through - most likely we'll want to create a wiby-my-feature in the dependent, but we will not start it at main / master, but at my-feature instead. In that case, there could be some DX wins if the PR on the dependent tried to merge wiby-my-feature into my-feature, but at the same time - if it tries to merge wiby-my-feature into main, it's probably no big deal either?

This is hard 😅

To simplify this - I think initially the base param should always be empty (if GH will accept that), and the rest should be addressed as we tackle the existing branches case.

Not quite done yet, I'm going for the merging into the default branch main or whatever the default is set to. ( the better DX experience needs another change which I'm going to try for tomorrow.

@dominykas , I altered the code to raise the PR against the default branch and added changes to the tests to cover this most simple scenario. I too want to get this PR merged and maybe covering the other edge cases should be handled in another follow up PR?

ghinks · 2021-04-18T12:56:49Z

I'm redoing this with the suggestions that it be a different feature flag.
I will incorporate the comments you made above.
The tough part is the new feature name

close-pr
auto-clean
...
always tough to give things names

I am going to merge the changes I have made back into this PR now. We may need to re-review it.

add new feature flag... for PR removal

add small amount of text output for success case with the intent of adding proper processing next.

lint fixes

updates to names and suggestions from feedback

add a TODO to remind me to create a processOutput for the close-pr command

dominykas · 2021-04-28T11:23:54Z

🤔 I see you added close-pr as a new command. I guess it might be useful on its own. 👍

That said, I think we probably still need to add it into wiby clean as well? Because wiby clean won't be able to delete branches if they have PRs open.

dominykas · 2021-04-28T11:59:42Z

lib/test.js

@@ -32,6 +32,9 @@ module.exports = async function ({ dependents }) {

    const patchedPackageJSON = applyPatch(parentDependencyLink, parentPkgJSON.name, dependentPkgJson, parentPkgJSON.name)
    await pushPatch(patchedPackageJSON, dependentRepositoryInfo.owner, dependentRepositoryInfo.name, parentPkgJSON.name, parentBranchName)
+    if (pullRequest) {


Updated #95 (comment) - if we think that's the right approach, then we should check if the PR is already open here (we need to open a new one if it's closed).

I can't recall if the GH API returns the PR list as part of the "get branch" API call, or if there's a separate API call for that...

Ok. I agree that clean needs it to. I'll merge the latest changes back into my branch and carry on.

One other thing occurred to me. We may want to be using the Promise.allSettled() module as if we checking upon multiple checks. But that can be for later.

Promise.allSettled()

Yeah, that probably makes sense.

After looking at the needs of the clean command. I do not think we need to do anything. If the branch is deleted during a clean then the PR that was raised against that branch to trigger an action is also deleted.

If you agree with my opinion I'll look at issue 95 with respect to the close-pr command.

add TODO noting that the run checks could be > 1 for a branch and that some could pass and some could fail and we need to handle that case

# Conflicts: # test/cli/test.js

bin/commands/close-pr.js

Co-authored-by: Dominykas Blyžė <[email protected]>

ghinks · 2021-05-12T18:18:45Z

many thanks for the reviews

dominykas · 2021-05-17T08:39:28Z

I'm keen to merge this, but #93 (comment) is not yet resolved? (the parentBranchName thread)

ghinks · 2021-05-17T10:14:59Z

I'll look at this with respect to #93 tomorrow. Today I need to prep or the talk for OpenJSWorld.

PR should be raised against the default branch this could be main. Next handle the case that the feature branch is already there.

…ture/close-draft-pr

test that the parent branch exists or not in the dependent. If the dependency parent branch exists in the dependent use that branch and raise a PR of wiby-feature-x into feature-x rather than into the default branch.

rationalize repeated nock code block into a beforeEach clause.

ghinks · 2021-05-22T17:35:19Z

@dominykas I had some extra time so I have addressed the case where the dependency feature branch name could be pre-existing in the dependent repo, and altered the PR raised to be wiby-feature-x into feature-x in that case.

dominykas · 2021-06-28T12:56:31Z

wiby test

dominykas · 2021-06-28T12:56:53Z

(don't really expect ☝️ to work, but still hoping that maybe GH fixed the issue I was having...)

dominykas · 2021-07-13T11:41:05Z

🎉 This PR is included in version 0.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

ghinks added 4 commits April 6, 2021 21:43

feat: raise PR on branch create

761feef

raise a PR when the --pullrequest=true or if set in the wiby.json file.

feat: raise PR on branch create

d1566eb

add a body and title to the PR

feat: remove PR on test success

16ec3e0

remove the PR during the result scan

feat: remove PR on test success

c514690

add tests for PR closing

ghinks marked this pull request as draft April 10, 2021 11:15

feat: remove PR on test success

18004c5

change the status and the conclussion to values defined in [checkrun definitio](https://docs.github.com/en/[email protected]/graphql/reference/objects#checkrun)

dominykas reviewed Apr 11, 2021

View reviewed changes

.wiby.json Outdated Show resolved Hide resolved

dominykas reviewed Apr 11, 2021

View reviewed changes

lib/github.js Outdated Show resolved Hide resolved

dominykas reviewed Apr 11, 2021

View reviewed changes

ghinks added 3 commits April 12, 2021 16:37

feat: remove PR on test success

ac689bb

change updatePRStatus to closePR

feat: remove PR on test success

a9f028d

change pullrequest to pull-request change pullRequest (json) to pullRequest

feat: remove PR on test success

562ace0

change the pullRequest to be per dependent object

rodion-arr reviewed Apr 16, 2021

View reviewed changes

lib/test.js Outdated Show resolved Hide resolved

dominykas reviewed Apr 18, 2021

View reviewed changes

lib/result.js Outdated Show resolved Hide resolved

dominykas reviewed Apr 18, 2021

View reviewed changes

ghinks added 6 commits April 18, 2021 09:02

feat: clean PR

c6138ce

add new feature flag... for PR removal

feat: clean PR

b07d4a7

add small amount of text output for success case with the intent of adding proper processing next.

feat: clean PR

e143667

lint fixes

feat: clean PR

49d752f

updates to names and suggestions from feedback

feat: clean PR

5218caf

add a TODO to remind me to create a processOutput for the close-pr command

Merge branch 'main' into feature/close-draft-pr

4900188

ghinks mentioned this pull request Apr 22, 2021

Existing branch handling #95

Closed

dominykas reviewed Apr 28, 2021

View reviewed changes

ghinks added 2 commits April 28, 2021 16:40

feat: clean PR

2c56d47

add TODO noting that the run checks could be > 1 for a branch and that some could pass and some could fail and we need to handle that case

Merge branch 'main' into feature/close-draft-pr

7cb66cb

# Conflicts: # test/cli/test.js

ghinks marked this pull request as ready for review May 5, 2021 10:43

rodion-arr approved these changes May 7, 2021

View reviewed changes

dominykas reviewed May 10, 2021

View reviewed changes

bin/commands/close-pr.js Outdated Show resolved Hide resolved

Update bin/commands/close-pr.js

0ddb7b4

Co-authored-by: Dominykas Blyžė <[email protected]>

ghinks added 4 commits May 19, 2021 16:29

feat: raise PR

ff793cd

PR should be raised against the default branch this could be main. Next handle the case that the feature branch is already there.

Merge remote-tracking branch 'origin/feature/close-draft-pr' into fea…

0a980e9

…ture/close-draft-pr

feat: raise PR

4bf337b

test that the parent branch exists or not in the dependent. If the dependency parent branch exists in the dependent use that branch and raise a PR of wiby-feature-x into feature-x rather than into the default branch.

feat: raise PR

ba96374

rationalize repeated nock code block into a beforeEach clause.

dominykas approved these changes Jun 28, 2021

View reviewed changes

dominykas merged commit b0fb895 into pkgjs:main Jun 28, 2021

dominykas mentioned this pull request Jun 28, 2021

Open pull-requests when testing #53

Closed

dominykas added the released label Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/close draft pr #93

Feature/close draft pr #93

ghinks commented Apr 10, 2021 •

edited

Loading

dominykas Apr 11, 2021 •

edited

Loading

ghinks Apr 12, 2021

dominykas commented Apr 11, 2021 •

edited

Loading

dominykas Apr 18, 2021

ghinks Apr 18, 2021

dominykas Apr 28, 2021

dominykas Apr 18, 2021

ghinks Apr 18, 2021

ghinks Apr 22, 2021

dominykas Apr 28, 2021 •

edited

Loading

dominykas May 17, 2021

ghinks May 19, 2021

ghinks May 22, 2021

ghinks commented Apr 18, 2021 •

edited

Loading

dominykas commented Apr 28, 2021

dominykas Apr 28, 2021

ghinks Apr 28, 2021 •

edited

Loading

dominykas Apr 28, 2021

ghinks Apr 29, 2021 •

edited

Loading

ghinks commented May 12, 2021

dominykas commented May 17, 2021

ghinks commented May 17, 2021

ghinks commented May 22, 2021

dominykas commented Jun 28, 2021

dominykas commented Jun 28, 2021

dominykas commented Jul 13, 2021

Feature/close draft pr #93

Feature/close draft pr #93

Conversation

ghinks commented Apr 10, 2021 • edited Loading

dominykas Apr 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominykas commented Apr 11, 2021 • edited Loading

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 Apr 28, 2021 • edited Loading

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

ghinks commented Apr 18, 2021 • edited Loading

dominykas commented Apr 28, 2021

Choose a reason for hiding this comment

ghinks Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghinks Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

ghinks commented May 12, 2021

dominykas commented May 17, 2021

ghinks commented May 17, 2021

ghinks commented May 22, 2021

dominykas commented Jun 28, 2021

dominykas commented Jun 28, 2021

dominykas commented Jul 13, 2021

ghinks commented Apr 10, 2021 •

edited

Loading

dominykas Apr 11, 2021 •

edited

Loading

dominykas commented Apr 11, 2021 •

edited

Loading

dominykas Apr 28, 2021 •

edited

Loading

ghinks commented Apr 18, 2021 •

edited

Loading

ghinks Apr 28, 2021 •

edited

Loading

ghinks Apr 29, 2021 •

edited

Loading