akuity / kargo

Application lifecycle orchestration
https://kargo.akuity.io/
Apache License 2.0
1.75k stars 145 forks source link

refactor: rewrite Stage reconciler #2905

Closed hiddeco closed 6 days ago

hiddeco commented 2 weeks ago

This refactors the logic of the "regular" Stage reconciler into smaller chunks, with more extensive test coverage.

In addition, it adds conditions to the Stage to allow a better assessment of the state it is in over the previous Phase:

netlify[bot] commented 2 weeks ago

Deploy Preview for docs-kargo-io ready!

Name Link
Latest commit 3e004308a753f6013a2a34f93c6f30b6f725a9e2
Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/6737c9d73ca6990008cb4e91
Deploy Preview https://deploy-preview-2905.docs.kargo.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.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 79.42318% with 371 lines in your changes missing coverage. Please review.

Project coverage is 50.86%. Comparing base (6e34eb0) to head (3e00430). Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
internal/controller/stages/regular_stages.go 75.94% 332 Missing and 23 partials :warning:
api/v1alpha1/stage_types.go 0.00% 10 Missing :warning:
internal/rollouts/analysis_template.go 97.10% 3 Missing and 1 partial :warning:
cmd/controlplane/controller.go 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2905 +/- ## ========================================== + Coverage 50.21% 50.86% +0.65% ========================================== Files 276 278 +2 Lines 24919 25113 +194 ========================================== + Hits 12512 12773 +261 + Misses 11718 11651 -67 Partials 689 689 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

krancour commented 6 days ago

One thing that tripped me up when reading through the changes, although this is not necessarily a new issue:

The method names verifyStageFreight() and verifyFreightForStage() are quite similar. The former has a good comment explaining what it does. The latter could probably stand to either be renamed or commented in a way that makes it more clear that it's about marking Freight as verified as opposed to performing verification.

hiddeco commented 6 days ago

The latter could probably stand to either be renamed or commented in a way that makes it more clear that it's about marking Freight as verified as opposed to performing verification.

I had this same concern, but at the time could not figure out what name would be better. "Marking" however is a good suggestion, will make the change.