-
Notifications
You must be signed in to change notification settings - Fork 172
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
feat: add ui stage link to promo prs #2753
Conversation
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2753 +/- ##
==========================================
+ Coverage 49.38% 49.50% +0.11%
==========================================
Files 270 271 +1
Lines 19521 19666 +145
==========================================
+ Hits 9640 9735 +95
- Misses 9255 9300 +45
- Partials 626 631 +5 ☔ View full report in Codecov by Sentry. |
Freight: *workingPromo.Status.FreightCollection.DeepCopy(), | ||
StartFromStep: promo.Status.CurrentStep, | ||
State: directives.State(workingPromo.Status.GetState()), | ||
APIServerBaseURL: r.cfg.APIServerBaseURL, |
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.
Nit (and non-blocking): this should probably become part of a nested something at some point. As its placement feels odd as-is.
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.
Second nit: from the perspective of what we use it for now (within the context of directives), isn't this just the UIBaseURL
?
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.
Fair point. I'll change that.
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.
Fixed in fd78be3.
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 should probably become part of a nested something at some point
This part, I'll save for another day, because I cannot quite figure out yet what other things it ought to be grouped with.
Fixes #1825