-
Notifications
You must be signed in to change notification settings - Fork 3
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
Change the behaviour of PR creation #24
Conversation
We now do: - if the user closed the PR, we won't do anything as we assume the PR was rejected - if the PR is opened, we update the branch by force-pushing - if the PR was not there at all, we open a new one Fixes: stacklok#14
@@ -225,6 +232,7 @@ func (fa *FrizbeeAction) createPR(ctx context.Context) error { | |||
Username: fa.RepoOwner, | |||
Password: fa.Token, | |||
}, | |||
Force: true, |
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.
Why the force here? Shouldn't a regular push work if the branch is new?
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 update the branch if an existing PR was opened. I'm fine removing this and I'm actually glad someone else than me or @rdimitrov joined the reviews, because we were discussing the workflow today and weren't sure what is more accepted by users.
Without force-push I guess the workflow would work like this:
- a PR is opened by this action
- someone pushes a new action into a workflow in a separate PR
- the original PR is merged
- on next tick of the action (either on schedule or on push to master) a new frizbee PR is created
With this patch:
- a frizbee PR is created
- another PR adds a new action
- the frizbee action is triggered either on schedule or on push to main and force-pushes to the branch, updating the existing PR
WDYT?
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.
Ah got it, thanks for the context. I thought you were trying to avoid touching any existing PRs. I got confused because I was expecting to see a rebase, and did not see the commit being recreated elsewhere.
SGTM
if existingPR != nil && existingPR.GetState() == "open" { | ||
fmt.Printf("PR %d already exists. Won't create a new one\n", existingPR.GetNumber()) | ||
return nil | ||
} |
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.
Since there is a force push before this, this block should probably be moved around line 213 to bail early and ensure NOOP.
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.
But then we wouldn't update an existing PR would we?
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 thank for the clarification above!
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.
lgtm!
We now do:
was rejected
Fixes: #14