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

feat: keep track of promotion state in promotion status #2604

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

krancour
Copy link
Member

@krancour krancour commented Sep 29, 2024

@hiddeco it is best for you to review this one.

I know we had several offline conversations about whether this was worth avoiding or something we'd be forced to do.

Waiting for a PR to merge or close is the thing I ended up really needing this for.

Going back and looking at legacy code, even then, we never really had a way of doing this without storing the PR number in promotion status' metadata field.

fwiw, this is the last thing standing in the way of all of these directive-based examples passing:

https://github.com/krancour/kargo-examples/tree/for-v0.9.0

If you are generally unopposed to this, but wish to change some implementation details, please feel free to amend this PR to maximize the possibility that we cut a release candidate early in the day tomorrow.

Edit: A tangential benefit of this is it provides a little more visibility into what step is holding up a long-running promotion.

@krancour krancour requested a review from a team as a code owner September 29, 2024 23:49
@krancour krancour added the priority/urgent Fix right away label Sep 29, 2024
Copy link

netlify bot commented Sep 29, 2024

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit e98b457
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66fa8d6ab5525200080459b6
😎 Deploy Preview https://deploy-preview-2604.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Sep 29, 2024

Codecov Report

Attention: Patch coverage is 26.37363% with 67 lines in your changes missing coverage. Please review.

Project coverage is 51.05%. Comparing base (022c431) to head (e98b457).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/directives/git_pr_waiter.go 6.66% 28 Missing ⚠️
internal/controller/promotions/promotions.go 0.00% 16 Missing ⚠️
internal/directives/simple_engine.go 66.66% 10 Missing and 1 partial ⚠️
api/v1alpha1/promotion_types.go 0.00% 7 Missing ⚠️
internal/directives/state.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2604      +/-   ##
==========================================
- Coverage   51.09%   51.05%   -0.04%     
==========================================
  Files         282      282              
  Lines       20961    21007      +46     
==========================================
+ Hits        10709    10726      +17     
- Misses       9584     9614      +30     
+ Partials      668      667       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

No real objection from me. I wonder if in the future we continue to want to persist all state to the object (and not e.g. a sub-set which has to survive), but I think this concern can be addressed later.

api/v1alpha1/promotion_types.go Outdated Show resolved Hide resolved
@krancour krancour merged commit 794cfbc into akuity:main Sep 30, 2024
14 of 17 checks passed
@krancour krancour deleted the krancour/preserve-promotion-state branch September 30, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants