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.

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

Don't attempt to reopen a PR after updating a branch if it already exists #1944

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Dec 15, 2023

The PR remediation was checking if a PR with the same contents already
exist as a fast and cheap check to avoid triggering the remediation
again. But in case the profile changes or the contents change for
another reason, we want to push to the existing branch to update it, but
don't try to open a PR again as that would fail with 422.

To do that, this patch adds a check if a PR is already open from the
branch we just pushed to and if it is, just returns before opening the
PR again.

@jhrozek jhrozek force-pushed the pr_branch_already_exists branch from 972974b to c3a3e17 Compare December 15, 2023 14:51
rdimitrov
rdimitrov previously approved these changes Dec 15, 2023
@JAORMX
Copy link
Contributor

JAORMX commented Dec 15, 2023

Oops! You need to update the unit tests with the new expected GH API calls:

❌ TestPullRequestRemediate/open_a_PR (190ms)
      pull_request.go:481: Unexpected call to *mockgh.MockGitHub.ListPullRequests([context.Background stacklok minder 0xc0006a6000]) at /home/runner/work/minder/minder/internal/engine/actions/remediate/pull_request/pull_request.go:481 because: 
          expected call at /home/runner/work/minder/minder/internal/engine/actions/remediate/pull_request/pull_request_test.go:197 has already been called the max number of times
      controller.go:269: missing call(s) to *mockgh.MockGitHub.CreatePullRequest(is anything, is equal to stacklok (string), is equal to minder (string), is equal to Add Dependabot configuration for gomod (string), is equal to <!-- minder: pr-remediation-body: { "ContentSha": "1041e57c2fac284bdb7827ce55c6e3cb609e97b9" } -->
          
          Adds Dependabot configuration for gomod (string), is equal to refs/heads/minder_add_dependabot_configuration_for_gomod (string), is equal to main (string)) /home/runner/work/minder/minder/internal/engine/actions/remediate/pull_request/pull_request_test.go:398
      controller.go:269: aborting test due to missing call(s)

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unit test updates needed

…ists

The PR remediation was checking if a PR with the same contents already
exist as a fast and cheap check to avoid triggering the remediation
again. But in case the profile changes or the contents change for
another reason, we want to push to the existing branch to update it, but
don't try to open a PR again as that would fail with 422.

To do that, this patch adds a check if a PR is already open from the
branch we just pushed to and if it is, just returns before opening the
PR again.
@jhrozek jhrozek merged commit 888f7cf into mindersec:main Dec 18, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants