Git-Open-PR fails when there is no change

[kaminski-dev]

Checklist

• [x] I've searched the issue queue to verify this is not a duplicate bug report.
• [x] I've included steps to reproduce the bug.
• [x] I've pasted the output of kargo version.
• [x] I've pasted logs, if applicable.

Description

Current behaviour:

• git-open-pr action fails when there are no changes between the stage branch and Kargo branch
• This causes the promotion pipeline to fail, even though there's nothing to promote

Desired behaviour:

• When there are no changes, git-open-pr should skip gracefully (not fail)
• git-wait-for-pr should also be skipped in this case
• This mirrors how git-commit behaves—if there's nothing to commit, it just skips

How our stages are configured:

• Multi-stage pipeline (DEV -> SIT -> UAT -> PROD) If we have changes which impact the customer we have the following agreement. We just change the values for the specific stage (for example UAT). This will create a new freight which will work until UAT. For PROD the git-open-pr action will fail since it can't create a PR because there are no changes. After few days when rolling out to PROD we will have the same behaviour but in UAT and now we need to manually approve the freight because it fails in UAT.

The above approach gives us the flexibility to rollout multiple different changes because if we would change all the values at once we still need to wait for a couple of days if there is no customer complaining. This would lead to the situation that co workers of mine would need wait in a queue for their changes to be rolled out.

The step should do following:

• Skip if nothing to do

Screenshots

Steps to Reproduce

• Create a pipeline with 3 stages (DEV, SIT, PROD)
• For SIT / PROD add the git-open-pr and git-wait-for-pr step
• Change only something in SIT
• New freight is created
• Promote the freight until PROD -- here you will face the issue since no PR can be created
• Change only something in PROD
• New freight is created
• Promote the freight until PROD -- it will fail in SIT since no PR can be created

Version

v1.7.5

Logs

step "pr" met error threshold of 1: error running step "pr": error creating pull request: POST https://my-code-repo/api/v3/repos/myorg/myrepo/pulls: 422 Validation Failed [{Resource:PullRequest Field: Code:custom Message:No commits between rsp-myrepo/uat-ha2 and kargo/promotion/uat-ha2-pulsar.01k7hgvw36pfdd9hkkemp05b1b.71e5a7e}]

[krancour]

I can see this perspective, but this will require some discussion among maintainers before any action is potentially taken on it. It requires multiple steps to have internal logic that infers what's been done by previous steps, which makes this seem substantially more "magical" than the git-commit step skipping the commit if there are no diffs, which is a very local decision.

In the interim, I can suggest that you might find some value in this section of the docs:

https://docs.kargo.io/user-guide/reference-docs/promotion-templates/#conditional-steps

[jessesuen]

I also faced the same issue. The workaround to this is with the following check to see if the commit step was skipped, and then skip the subsequent PR steps if it was. e.g.:

  - uses: git-clone
    config:
      repoURL: ${{ vars.repoURL }}
      checkout:
      - branch: main
        path: ./src
...
  - uses: git-commit
    as: commitStep
    config:
      path: ./src

  - if: ${{ status('commitStep') != 'Skipped' }}
    uses: git-open-pr
...

  - if: ${{ status('commitStep') != 'Skipped' }}
    uses: git-wait-for-pr
    config:
      repoURL: ${{ vars.repoURL }}
      prNumber: ${{ task.outputs['open-pr'].pr.id }}

~I know it's a bit clunky, but it works.~

~@krancour is it possible for a step to check if another was skipped? So instead of this:~

if: ${{ task.outputs.checkout.commits['./src'] != task.outputs.commitStep.commit }}

~We could do something simpler?~

if: ${{ taskStatus('commitStep') == 'Skipped' }}

EDIT: we indeed have a status() function. I've updated the workaround.

[jessesuen]

It requires multiple steps to have internal logic that infers what's been done by previous steps

I think we can achieve the use case while keeping decisioning to be local. If I were to change the behavior of git-open-pr and git-wait-for-pr git-merge-pr, we could do something like the following:

1. git-open-pr would be marked Skipped if GitHub/GitLab/etc... rejected the PR because there were no changes.
2. git-wait-for-pr and git-merge-pr would allow for prNumber to be empty. If prNumber was supplied to be empty, the step would be marked Skipped.

[krancour]

@jessesuen, I had the same thought for 1. 2 was where I thought things get tricky, but your idea is a good one.

Re: checking status of another a previous step, yes. It's described in the doc I linked to.

[krancour]

It's the status() function.

[kaminski-dev]

Thanks for the Feedback @jessesuen / @krancour.

One of my coworkers already commented on a solution we use in some teams, see the comment from WZHGAHO in #3819.

Could you provide feedback on our workaround? We use a copy promotion step to deploy a YAML template, then fill it with values using yaml-update, including a timestamp field set to ${{ now() }}. This ensures there's always a change to promote.

We'd appreciate a native solution, it could be an automatic timestamp update or something which you think makes sense. It just should not break if there is no change. If we can support you with any feedback or brainstorming, we're happy to help on this topic.

I'll also review the conditional skip workaround based on commit status, but a native solution would be preferable.

[krancour]

@kaminski-dev I don't have strong opinions about that workaround. I think using conditional steps is cleaner though, but that's a matter of opinion. I agree a native solution is ideal. I just want to be very deliberate in the approach.

(You'll notice a theme lately of me pumping the brakes by promoting discussion. There have been a lot of enthusiastic new contributors and what I really want to do is have (senior) maintainer buy-in on approaches to things before people dive into implementations. We're just trying to avoid wasted effort and especially trying to avoid short-sighted solutions that might force a breaking change on us when we correct/improve it at a later date.)

Re-visiting @jessesuen's suggestions from yesterday:

git-open-pr is easy to enhance such that lack of diffs between two branches or certain errors when attempting to open the PR result in the step being "skipped." No further discussion required on this.

git-wait-for-pr (and git-merge-pr) will be harder. I like @jessesuen's suggestion of allowing the prNumber field to be empty and those steps can skip themselves when it is. My only concern is whether it's a foot gun (or how much of one it is). Allowing prNumber to be empty means that field becomes optional. If it's no longer required, I can easily imagine users forgetting to provide it. Today, validation would catch that and the user would understand very clearly what they've done wrong and would be able to correct it pretty quickly. Once that's no longer a validation failure, I can foresee it leading to a very wrong assumption on the part of users who didn't set it -- that there's magic involved that infers the PR number from some previous step (there isn't, nor do I want that).

I'm thinking through the consequences of such a mistake. If a previous step had opened a PR, then a step intended to wait for it were accidentally configured without any value in the prNumber field, the wait step would skip itself and the promotion would continue on to steps meant to occur only after the PR was merged. Depending on what those steps are, you might end up with some very surprising outcomes. Suppose the next step syncs an Argo CD App to the head of the branch that the PR has not yet been merged into. Now what's running in that environment is neither what you think it is nor what you wanted it to be.

I still like the idea. I just want to figure out whether we need to put some guard rails around this, and if so, I want to figure out what those need to look like.

[krancour]

Assigning @fykaa, but @fykaa do not implement anything until we have a clear plan and have determined what sort of priority this is.

[kaminski-dev]

Hi @krancour , just wanted to ask if you already determined the priority of this issue?

[jessesuen]

Allowing prNumber to be empty means that field becomes optional. If it's no longer required, I can easily imagine users forgetting to provide it. Once that's no longer a validation failure, I can foresee it leading to a very wrong assumption on the part of users who didn't set it

For this concern, we should never allow prNumber to be omitted. git-wait-for-pr and git-merge-pr should still fail with validation error if the field is left unspecified. However, the change we would make is: if that happens to resolve to empty string at runtime, then we skip the step.

If we agree to that behavior, I think this should be a easy change, and so targetting v1.9

[jessesuen]

Allowing prNumber to be empty means that field becomes optional

OH, are you saying that we can't actually tell the difference between a user omitting it from the PromotionTask spec, e.g.:

    uses: git-wait-for-pr
    config:
      repoURL: ${{ vars.repoURL }}

versus when it resolves to empty string at runtime?

    uses: git-wait-for-pr
    config:
      repoURL: ${{ vars.repoURL }}
      prNumber: ${{ task.outputs['open-pr'].pr?.id ?? "" }}

My idea was that the first would result in validation error. But the second would be accepted.

However, thinking about this more, perhaps we only should enhance git-open-pr to result in Skipped if there are no changes, and have subsequent steps check if it was skipped. e.g.:

  - uses: git-commit
    config:
      path: ./src

  - uses: git-open-pr
    as: open-pr
...

  - if: ${{ status('open-pr') != 'Skipped' }}
    uses: git-wait-for-pr
    config:
      repoURL: ${{ vars.repoURL }}
      prNumber: ${{ task.outputs['open-pr'].pr.id }}

That might be clearer, more explicit, than using something like nil coalencing in the prNumber:

  - uses: git-commit
    config:
      path: ./src

  - uses: git-open-pr
    as: open-pr
...

  - uses: git-wait-for-pr
    config:
      repoURL: ${{ vars.repoURL }}
      prNumber: ${{ task.outputs['open-pr'].pr?.id ?? "" }}