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

fix(controller): improve logic to determine when syncing an argo cd app is required #2433

Merged
merged 2 commits into from
Aug 15, 2024

Conversation

krancour
Copy link
Member

Fixes #2427

925b765 (the second commit) is the actual bug fix and is pretty straightforward. It modifies the sync operations we initiate to include the id of the Freight collection that is being promoted. We check for this information when determining if we need to initiate a sync operation. This allows a promotion to wait for a sync operation to complete in cases where we previously would not have simply for lack of any means to determine whether the completed sync operation was indeed the one we'd been waiting for.

fe15116 (the first commit) changes the interface of promotion mechanisms to be quite a bit cleaner. The bug fix described above required the Argo CD app update promotion mechanism to have access to the Freight collection ID. It could obtain it from promo.Status.FreightCollection.ID. Doing this made me realize how seemingly redundant it was that promotion mechanisms were also receiving and returning []kargoapi.FreightReference, since there is already access to the Freight collection. In fact, it was not entirely redundant because promotion mechanisms can make certain modifications to those Freight references (like set health check commit IDs) and return them, and then those modified references become the input to the next promotion mechanism. The situation was quite similar for Promotion status, which could be modified by each mechanism and returned. The more I looked at this, the more confusing it started to look and feel, and I felt it was leaving too much potential for future mistakes. So, while I don't usually love when functions modify an argument in place (Promotion, in this case), updating the promotion mechanisms to get all details from the Promotion argument and modify its status (including Freight collection) in place made everything feel much more straightforward.

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
@krancour krancour added this to the v0.8.5 milestone Aug 15, 2024
@krancour krancour self-assigned this Aug 15, 2024
@krancour krancour requested a review from a team as a code owner August 15, 2024 16:07
Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for docs-kargo-akuity-io canceled.

Name Link
🔨 Latest commit 925b765
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/66be27e06e6f0c000818b1af

@krancour krancour changed the title Krancour/bug fix fix(controller): improve logic to determine when syncing an argo cd app is required Aug 15, 2024
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 68.25397% with 40 lines in your changes missing coverage. Please review.

Project coverage is 48.48%. Comparing base (9f24ba3) to head (925b765).
Report is 8 commits behind head on main.

Files Patch % Lines
internal/controller/promotion/pullrequest.go 0.00% 17 Missing ⚠️
internal/controller/promotions/promotions.go 0.00% 12 Missing ⚠️
internal/controller/promotion/composite.go 84.84% 3 Missing and 2 partials ⚠️
internal/controller/promotion/git.go 79.16% 5 Missing ⚠️
internal/controller/promotion/argocd.go 97.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2433      +/-   ##
==========================================
+ Coverage   48.38%   48.48%   +0.10%     
==========================================
  Files         245      245              
  Lines       17761    17757       -4     
==========================================
+ Hits         8593     8610      +17     
+ Misses       8748     8725      -23     
- Partials      420      422       +2     

☔ 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.

This is absolutely better, and much more resilient than what I came up with. 💯 for thinking about the operation info field! 🧠🥇

@krancour krancour added this pull request to the merge queue Aug 15, 2024
Merged via the queue into akuity:main with commit af685c3 Aug 15, 2024
23 of 25 checks passed
@krancour krancour deleted the krancour/bug-fix branch August 15, 2024 22:51
github-actions bot pushed a commit that referenced this pull request Aug 16, 2024
…pp is required (#2433)

Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
(cherry picked from commit af685c3)
@akuitybot
Copy link

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.

bug: not checking if app sources are synced correctly in some common cases
3 participants