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

git-open-pr : "Another open merge request already exists for this source branch" : retrieve the ID of the MR/PR and go next step #2845

Closed
1 of 3 tasks
flbla opened this issue Oct 25, 2024 · 8 comments · Fixed by #2896

Comments

@flbla
Copy link

flbla commented Oct 25, 2024

Checklist

  • I've searched the issue queue to verify this is not a duplicate feature request.
  • I've pasted the output of kargo version, if applicable.
  • I've pasted logs, if applicable.

Proposed Feature

When a MR is already opened retrieve it's ID and continue with next step

Motivation

Stage is failing but it should not

@Brightside56
Copy link
Contributor

same on 0.9.1

@krancour krancour self-assigned this Oct 25, 2024
@krancour krancour added this to the v1.0.4 milestone Oct 25, 2024
@krancour
Copy link
Member

@hiddeco, give me a sanity check?

The legacy promo mechs in v0.9.x and earlier used to store the promotion number in the Promotion's bygone metadata map -- a precursor to promotion steps' shared state. It had to do this, because the legacy promo mechs replayed in their entirety on every reconciliation and there had to be some way of knowing the PR in question had already been opened.

For reference:

if getPullRequestNumberFromMetadata(promo.Status.Metadata, update.RepoURL) == -1 {
// PR was never created. Prepare the branch for the commit
if err = preparePullRequestBranch(repo, commitBranch, update.WriteBranch); err != nil {
return fmt.Errorf("error preparing PR branch %q: %w", update.RepoURL, err)
}
}

We currently have no equivalent to this check in the git-open-pr step and that is what needs to be rectified to resolve this issue.

Promotion steps only repeat themselves if we're forced to start a Promotion over from the beginning because we lost its working directory -- which would pretty much only be because the controller restarted while the Promotion was in-progress.

You can see this right here:

promoCtx := directives.PromotionContext{
UIBaseURL: r.cfg.APIServerBaseURL,
WorkDir: filepath.Join(os.TempDir(), "promotion-"+string(workingPromo.UID)),
Project: stageNamespace,
Stage: stageName,
FreightRequests: stage.Spec.RequestedFreight,
Freight: *workingPromo.Status.FreightCollection.DeepCopy(),
StartFromStep: promo.Status.CurrentStep,
State: directives.State(workingPromo.Status.GetState()),
}
if err := os.Mkdir(promoCtx.WorkDir, 0o700); err == nil {
// If we're working with a fresh directory, we should start the promotion
// process again from the beginning.
promoCtx.StartFromStep = 0
promoCtx.State = nil
} else if !os.IsExist(err) {
return nil, fmt.Errorf("error creating working directory: %w", err)
}

You'll notice we clear the shared state if we determine we're starting over. If we reset the starting step back to zero, but don't clear the shared state right here, the git-open-pr step would be able to self-determine that it's already done what it needs to do. (Perhaps, for good measure, we update the PR in this case in case any previous step that has now re-played yielded different results than the first time through. Such a thing could happen, for instance, with Helm charts that render anything that is random or time-based.)

I cannot think of any reason not to make this change, but as I said... I wanted a quick sanity check. lmk

@hiddeco
Copy link
Contributor

hiddeco commented Oct 26, 2024

I think there are two scenarios we need to consider:

  1. The scenario you described with Promotion replays due to a lost working directory.

  2. A second variation where a previous Promotion failed/terminated after PR creation, which can happen because:

    • Users can use static branches for PRs
    • Our branch names are tied to Project/Stage name, not specific Promotions
      (I'm not sure how we ended up with 2, as my initial idea was Promotion-specific branches)

I agree with not clearing the shared state initially during step zero resets. But once the process has started, I have concerns about ending up with a mixed bag of "previous" and "new" attempted state, which arguably can be misleading in certain scenarios. An alternative to address this would be to determine if any garbage collection / finalization needs to happen based on the existing state, which would allow you to e.g. close the pull request and/or delete the branch before kickstarting a new promotion.

If we decide to find existing branches / pull requests / merge requests, we must enforce the state of the branch to match the Promotion steps' output.

@krancour
Copy link
Member

krancour commented Oct 26, 2024

  • Users can use static branches for PRs
  • Our branch names are tied to Project/Stage name, not specific Promotions
    (I'm not sure how we ended up with 2, as my initial idea was Promotion-specific branches)

Well... that's my mistake. Precisely because state can reset, I wanted to be sure we could deterministically derive a PR's head branch name. Why I based that on Project and Stage names instead of Promotion names... I can't say where my head was at in that moment. I will happily fix this.

I have concerns about ending up with a mixed bag of "previous" and "new" attempted state, which arguably can be misleading in certain scenarios.

I did think about that, but convinced myself that because steps are ordered, as long as state returned from any step overwrites state it returned from a previous execution, then everything any subsequent step might refer to is guaranteed to be based on fresh state. So I think any confusion would be limited to human confusion, but if you were ever to see state from a step greater than the current step, it would be obvious what had happened.

If we decide to find existing branches / pull requests / merge requests, we must enforce the state of the branch to match the Promotion steps' output.

Agree. And that would mostly happen naturally. If the first run through pushed to branch x and opened PR 42, the second run through would push to branch x (if there were any diffs) and leave PR 42 alone.

Edit: The only reason to update the PR may be if the title/body had changed for any reason.

Edit 2: I think it's both easier and, more importantly, more straightforward to "reclaim" branches and PRs from a previous execution than to garbage collect.

@BenjaminNeale-Heritage
Copy link

BenjaminNeale-Heritage commented Oct 29, 2024

In Kargo Render it was possible to either open a new PR or add to an existing PR that was already open for the Rendered Manifest Pattern branch. This is useful if the PR identifies an issue that needs to be fixed before the PR can be approved. It doesn't appear as if Kargo (v1.0.3) offers a similar option. From what I can tell, if a PR is opened by Kargo and then a new promotion is started to that stage from the same warehouse then the second promotion waits and does not start executing (refer image below).

Does this ticket refer to a scenario where the second promotion would be allowed to proceed to add the commits into the PR from the first promotion or is this ticket a completely different scenario?

image

@Brightside56
Copy link
Contributor

In Kargo Render it was possible to either open a new PR or add to an existing PR that was already open for the Rendered Manifest Pattern branch.

With promotion mechanisms stage with PR promotion doesn't have PR icon (at least in 0.9) and any other differences from stages without PR promotion. It can be hard to understand at all for user that PR is created and should me merged somewhere

@BenjaminNeale-Heritage
Copy link

With promotion mechanisms stage with PR promotion doesn't have PR icon (at least in 0.9) and any other differences from stages without PR promotion. It can be hard to understand at all for user that PR is created and should me merged somewhere

I think that this has been fixed in Kargo v1.0.3 (refer first image below). The first stage has the git-open-pr step while the second stage doesn't have this step. When there is an open PR that has been created and the stage is waiting for the PR to be merged via the git-wait-for-pr step then the cog icon continues spinning to show that the pipeline for that stage remains active.

In my comment above, I initiated another promotion while the original promotion was waiting for the open PR to be merged which led to the second promotion being queued. I left the PR open overnight (and it remains open) but both the active and pending promotion pipelines have now failed with the following message (refer second screenshot below):

failed to run step "git-wait-for-pr": error getting PR number: no PR number found in output from step with alias "open-pr"

This may be intended behaviour of Kargo but I wanted to mention it as it continues from my comment above. It does highlight though that PRs must be processed fairly quickly after being created by Kargo.

image
image

@flbla
Copy link
Author

flbla commented Nov 4, 2024

Hi,
In my case : I launched a stage, it openned a PR it failed some steps after, I fixed, relaunched and it failed at the open pr step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants