-
Notifications
You must be signed in to change notification settings - Fork 314
feat: show PR approval link for git-wait-for-pr steps while running #5560
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
base: main
Are you sure you want to change the base?
feat: show PR approval link for git-wait-for-pr steps while running #5560
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note that this requires the fix in #5559 for full effect. Here's some screenshots of this working:
In this example, the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5560 +/- ##
==========================================
+ Coverage 55.99% 56.01% +0.02%
==========================================
Files 423 423
Lines 32131 32148 +17
==========================================
+ Hits 17991 18008 +17
Misses 13087 13087
Partials 1053 1053 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| const isStatusAcceptable = isGitOpenPr | ||
| ? hasPullRequestStepSucceeded | ||
| : isGitWaitForPr | ||
| ? hasPullRequestStepSucceeded || hasPullRequestStepRunning | ||
| : false; |
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.
I think this is the same as:
const isStatusAcceptable =
(isGitOpenPr && hasPullRequestStepSucceeded) ||
(isGitWaitForPr && (hasPullRequestStepSucceeded || hasPullRequestStepRunning));
Which I personally find easier to read than the ternary version.
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.
Agreed, and updated!
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.
Looks like you forgot to push this change @DavidS-ovm ?
d46df5b to
772be2e
Compare
hiddeco
left a comment
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.
The backend side of this looks good to me. Thank you @DavidS-ovm 🙇
@Marvin9 will check the frontend bit to ensure it's backwards compatible and doesn't break when there are stale git-wait-for-pr steps without metadata.
| // Steps that can produce PR deep links | ||
| const PR_STEP_TYPES = ['git-open-pr', 'git-wait-for-pr']; |
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.
this is re-used in next file (but copied). I would suggest please move this constant in get-pr-link.ts
Marvin9
left a comment
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.
Please check backward compatibility. The link looks stale on git wait step without this output schema.
Previously, the "Waiting for Approval" component only appeared when promotion steps had a 'Succeeded' status and only for git-open-pr steps. This created a poor user experience for git-wait-for-pr steps, when they're not in the same stage as git-open-pr. They did not have a PR url attached and remain in 'Running' status while waiting for PR approval or merge. The fix differentiates between step types: - git-open-pr: Link appears only when step succeeds (PR is created) - git-wait-for-pr: Link appears when step is running or succeeded (PR exists and is waiting for approval/merge) This ensures users can access the PR for review/approval as soon as it's available, improving the GitOps workflow efficiency. WHY: Users need immediate access to the PR link during the approval waiting period, not just after the step completes. The PR URL is already available from the preceding git-open-pr step output, so there's no reason to hide the link until the wait step succeeds. Signed-off-by: David Schmitt <[email protected]>
772be2e to
ea4a139
Compare
|
Yup, it'd have helped if I acually pushed that change. I now did so with changes addressing all comments:
|


Previously, the "Waiting for Approval" component only appeared when promotion steps had a 'Succeeded' status and only for git-open-pr steps. This created a poor user experience for git-wait-for-pr steps, when they're not in the same stage as git-open-pr. They did not have a PR url attached and remain in 'Running' status while waiting for PR approval or merge.
The fix differentiates between step types:
This ensures users can access the PR for review/approval as soon as it's available, improving the GitOps workflow efficiency.
WHY: Users need immediate access to the PR link during the approval waiting period, not just after the step completes. The PR URL is already available from the preceding git-open-pr step output, so there's no reason to hide the link until the wait step succeeds.