Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/close draft pr #93
Feature/close draft pr #93
Changes from 14 commits
761feef
d1566eb
16ec3e0
c514690
18004c5
ac689bb
a9f028d
562ace0
c6138ce
b07d4a7
e143667
49d752f
5218caf
4900188
2c56d47
7cb66cb
0ddb7b4
ff793cd
0a980e9
4bf337b
ba96374
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that probably makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set the
base
toparentBranchName
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 pointparentBranchName
likely does not even exist - we should probably keep itundefined
(if the API allows it - otherwise we need to fetch the default branch name...)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure on this comment, but its late afternoon so I'm taking it easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's kind of related to that issue, but that's not exactly it.
When opening a PR, we want to merge
head
intobase
. So most of the timebase
should bemain
/master
(i.e. default branch), notparentBranchName
?my-feature
wiby test
wiby-my-feature
branch is created in the dependent repowiby-my-feature
intomain
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 awiby-my-feature
in the dependent, but we will not start it atmain
/master
, but atmy-feature
instead. In that case, there could be some DX wins if the PR on the dependent tried to mergewiby-my-feature
intomy-feature
, but at the same time - if it tries to mergewiby-my-feature
intomain
, it's probably no big deal either?This is hard 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?