Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change the behaviour of PR creation #24

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Change the behaviour of PR creation #24

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions pkg/action/action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,21 +194,28 @@ func (fa *FrizbeeAction) commitChanges(path, content string) error {
func (fa *FrizbeeAction) createPR(ctx context.Context) error {
// Create a new branch for the PR
branchName := "frizbee-action-patch"
var existingPR *github.PullRequest

// Check if a PR already exists for the branch and return if it does
openPrs, _, err := fa.Client.PullRequests.List(ctx, fa.RepoOwner, fa.RepoName, &github.PullRequestListOptions{
State: "open",
Head: branchName,
State: "all",
})
if err != nil {
return err
}
for _, pr := range openPrs {
if pr.GetHead().GetRef() == branchName {
fmt.Printf("PR %d already exists\n", pr.GetNumber())
return nil
existingPR = pr
break
}
}

if existingPR != nil && existingPR.GetState() == "closed" && branchExists(ctx, fa.Client, fa.RepoOwner, fa.RepoName, branchName) {
log.Printf("PR %d is closed. Won't do anything as long as branch %s exists", existingPR.GetNumber(), branchName)
return nil
}

headRef, err := fa.Repo.Head()
if err != nil {
log.Fatalf("failed to get head reference: %v", err)
Expand All @@ -225,6 +232,7 @@ func (fa *FrizbeeAction) createPR(ctx context.Context) error {
Username: fa.RepoOwner,
Password: fa.Token,
},
Force: true,
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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

RefSpecs: []config.RefSpec{
config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branchName, branchName)),
},
Expand All @@ -235,6 +243,12 @@ func (fa *FrizbeeAction) createPR(ctx context.Context) error {

fmt.Printf("Branch %s pushed successfully\n", branchName)

if existingPR != nil && existingPR.GetState() == "open" {
fmt.Printf("PR %d already exists. Won't create a new one\n", existingPR.GetNumber())
return nil
}
Comment on lines +246 to +249
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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!

// either the PR doesn't exist or was merged and it's time for another one

// Create a new PR
pr, _, err := fa.Client.PullRequests.Create(ctx, fa.RepoOwner, fa.RepoName, &github.NewPullRequest{
Title: github.String("Frizbee: Pin images and actions to commit hash"),
Expand All @@ -249,3 +263,8 @@ func (fa *FrizbeeAction) createPR(ctx context.Context) error {
fmt.Printf("PR %d created successfully\n", pr.GetNumber())
return nil
}

func branchExists(ctx context.Context, cli *github.Client, owner, repo, branch string) bool {
_, _, err := cli.Repositories.GetBranch(ctx, owner, repo, branch, 64)
return err == nil
}