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: add Freight to PromotionStatus #1721

Merged
merged 5 commits into from
Apr 23, 2024

Conversation

phuongdnguyen
Copy link
Contributor

@phuongdnguyen phuongdnguyen commented Apr 1, 2024

Fixes #1628

@phuongdnguyen phuongdnguyen requested a review from a team as a code owner April 1, 2024 10:58
Copy link

netlify bot commented Apr 1, 2024

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

Name Link
🔨 Latest commit ba3004d
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/6626946626be440008426e0f
😎 Deploy Preview https://deploy-preview-1721.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.

@phuongdnguyen
Copy link
Contributor Author

@krancour can you kindly take a look :)

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 46.99%. Comparing base (b3b1277) to head (ba3004d).

Files Patch % Lines
api/v1alpha1/promotion_types.go 0.00% 4 Missing ⚠️
internal/controller/promotions/promotions.go 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1721      +/-   ##
==========================================
- Coverage   47.00%   46.99%   -0.02%     
==========================================
  Files         219      219              
  Lines       14332    14337       +5     
==========================================
  Hits         6737     6737              
- Misses       7295     7300       +5     
  Partials      300      300              

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

@hiddeco
Copy link
Contributor

hiddeco commented Apr 9, 2024

@jacobnguyenn as this pull request would be a very welcome addition for some other things I want to factor out of the Promotion reconciler, I was wondering if are still interested in incorporating the feedback from #1721 (review).

I would otherwise also be happy to do the last bits for you, but wanted to give you a chance to address things yourself. (This doesn't have to be tomorrow, I can wait a little longer! 🍒)

@phuongdnguyen
Copy link
Contributor Author

@jacobnguyenn as this pull request would be a very welcome addition for some other things I want to factor out of the Promotion reconciler, I was wondering if are still interested in incorporating the feedback from #1721 (review).

I would otherwise also be happy to do the last bits for you, but wanted to give you a chance to address things yourself. (This doesn't have to be tomorrow, I can wait a little longer! 🍒)

thanks @hiddeco , i have been busy the last few days. I will get back to this today :)

@@ -96,6 +96,8 @@ type PromotionStatus struct {
// Metadata holds arbitrary metadata set by promotion mechanisms
// (e.g. for display purposes, or internal bookkeeping)
Metadata map[string]string `json:"metadata,omitempty" protobuf:"bytes,3,rep,name=metadata" protobuf_key:"bytes,1,opt,name=key" protobuf_val:"bytes,2,opt,name=value"`
// PromotedFreight is the detail of the piece of freight that was referenced by this promotion.
PromotedFreight *FreightReference `json:"promotedFreight,omitempty" protobuf:"bytes,4,opt,name=promotedFreight"`
Copy link
Contributor

Choose a reason for hiding this comment

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

On the fence about the naming of this field, as it is also set when the promotion has not completed and/or failed. While the current name may make you think it's the result of a successful promotion.

Given this, I wonder if it maybe should just be named Freight?

cc: @krancour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. FreightRef maybe?

Copy link
Member

Choose a reason for hiding this comment

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

It would be fine to just call it Freight. I don't think there would be any ambiguity in doing so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

})
if err != nil {
return nil, err
}

newStatus, nextFreight, err := r.promoMechanisms.Promote(ctx, stage, &promo, targetFreightRef)
Copy link
Contributor

@hiddeco hiddeco Apr 10, 2024

Choose a reason for hiding this comment

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

If I recall correctly, Promote may actually modify the FreightRef with more accurate information based on the outcome of the promotion mechanisms. Given this, I think only patching the Status before promoting doesn't suffice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please kindly help to point out where it was mentioned?

Copy link
Contributor

Choose a reason for hiding this comment

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

An example of where this happens is during Git promotion, where a HealthCheckCommit gets added to the FreightRef:

if commitIndex > -1 && newStatus.Phase == kargoapi.PromotionPhaseSucceeded {
newFreight.Commits[commitIndex].HealthCheckCommit = commitID
}

Copy link
Member

Choose a reason for hiding this comment

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

Spot on. The health check commit is the main thing that may change in the course of promotion.

To be perfectly honest, it's not going to matter to any health check logic if we don't update it the PromotionStatus, but it may cause confusion among human users if they notice the discrepancy between the FreightReference held by the Stage's CurrentFreight and History fields and this one.

Copy link
Member

Choose a reason for hiding this comment

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

i.e. So we should make sure it gets set here.

@phuongdnguyen phuongdnguyen force-pushed the promo-add-freight-ref branch from 18f19ef to 63beae7 Compare April 10, 2024 15:58
@phuongdnguyen phuongdnguyen changed the title feat: add FreightReference to PromotionStatus feat: add Freight to PromotionStatus Apr 10, 2024
@@ -119,7 +119,7 @@ func (g *gitMechanism) Promote(
); err != nil {
return nil, newFreight, err
}
newStatus = aggregateGitPromoStatus(newStatus, *otherStatus)
newStatus = aggregateGitPromoStatus(newStatus, *otherStatus).WithFreight(&newFreight)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure setting this here is the best thing we can do. As this would mean that for every promotion mechanism which does something with the Freight, we would have to remember to include it in the status.

What I would do instead, is persist the FreightReference to the PromotionStatus within internal/controller/promotions/promotions.go after r.promoMechanisms.Promote has been called. As nextFreight will contain the updated Freight data.

Copy link
Contributor Author

@phuongdnguyen phuongdnguyen Apr 15, 2024

Choose a reason for hiding this comment

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

What I would do instead, is persist the FreightReference to the PromotionStatus within internal/controller/promotions/promotions.go after r.promoMechanisms.Promote has been called. As nextFreight will contain the updated Freight data.

I see, but is it more convenient to reflect freight reference changes right beside the place we update its details? Also, it is pretty unclear which promotion mechanism caused freight reference update even if we update PromotionStatus in nternal/controller/promotions/promotions.go

Copy link
Contributor

@hiddeco hiddeco Apr 15, 2024

Choose a reason for hiding this comment

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

I see, but is it more convenient to reflect freight reference changes right beside the place we update its details?

Can you eleborate more on this question, I am not sure I am following the details of it.

Also, it is pretty unclear which promotion mechanism caused freight reference update even if we update PromotionStatus in internal/controller/promotions/promotions.go

Who and/or why would we have the desire to know what promotion mechanism changed something? Or how is the current approach solving this better?

Copy link
Contributor Author

@phuongdnguyen phuongdnguyen Apr 15, 2024

Choose a reason for hiding this comment

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

I see, but is it more convenient to reflect freight reference changes right beside the place we update its details?

Can you eleborate more on this question, I am not sure I am following the details of it.

I'm just wondering if it is more clear to add the new detail of FreightReference to the Promotion Status. In that way, after calling r.promoMechanisms.Promote, the caller will have the latest FreightReference data.

Also, it is pretty unclear which promotion mechanism caused freight reference update even if we update PromotionStatus in internal/controller/promotions/promotions.go

Who and/or why would we have the desire to know what promotion mechanism changed something? Or how is the current approach solving this better?

For the second part, since we're doing an aggregation of promote mechanisms, will it be useful (or not) to know that which promote mechanism changed the FreightReference detail.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the easiest place to assign the FreightReference to the PromotionStatus would be right here:

newStatus, nextFreight, err := r.promoMechanisms.Promote(ctx, stage, &promo, targetFreightRef)
if err != nil {
return nil, err
}

A superior, but more involved option might be to modify the signature of the Mechanism interface's Promote function so that promotion mechanisms do not return PromotionStatus and modified FreightReference separately, but just return the PromotionStatus with the FreightReference already set.

I think I would suggest taking the easier approach and either @hiddeco or myself will do the rest of the housekeeping in a follow-up.

@phuongdnguyen phuongdnguyen force-pushed the promo-add-freight-ref branch 2 times, most recently from f3439a0 to 2fa75cd Compare April 18, 2024 13:25
@hiddeco hiddeco force-pushed the promo-add-freight-ref branch from e42ce65 to 4cfa596 Compare April 18, 2024 14:27
@phuongdnguyen phuongdnguyen force-pushed the promo-add-freight-ref branch from b5c8e05 to 4feb10d Compare April 19, 2024 00:39
@krancour
Copy link
Member

@jacobnguyenn don't worry about conflicts. I'll run codegen tomorrow to resolve them then look at this with fresh eyes.

I think this might be all set.

@phuongdnguyen
Copy link
Contributor Author

@jacobnguyenn don't worry about conflicts. I'll run codegen tomorrow to resolve them then look at this with fresh eyes.

I think this might be all set.

Thanks for your patience 🎉

@krancour
Copy link
Member

Just needs codegen rerun and then this is 🚀

I will square that away on Monday.

@hiddeco hiddeco force-pushed the promo-add-freight-ref branch from 4feb10d to 7a4d28a Compare April 22, 2024 12:49
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.

Resolved the conflicts, this LGTM. Thanks @jacobnguyenn 🙇

phuongdnguyen and others added 5 commits April 22, 2024 18:39

Verified

This commit was signed with the committer’s verified signature.
hiddeco Hidde Beydals
Signed-off-by: Nguyen Duy Phuong <knowledge.phuongnguyen@gmail.com>

Verified

This commit was signed with the committer’s verified signature.
hiddeco Hidde Beydals
…m, add conventional method to add freight to promotion status

Signed-off-by: Nguyen Duy Phuong <knowledge.phuongnguyen@gmail.com>

Verified

This commit was signed with the committer’s verified signature.
hiddeco Hidde Beydals
Co-authored-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Nguyen Duy Phuong <knowledge.phuongnguyen@gmail.com>

Verified

This commit was signed with the committer’s verified signature.
hiddeco Hidde Beydals
Signed-off-by: Phuong D. Nguyen <knowledge.phuongnguyen@gmail.com>

Verified

This commit was signed with the committer’s verified signature.
hiddeco Hidde Beydals
Co-authored-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: Phuong D. Nguyen <knowledge.phuongnguyen@gmail.com>
@hiddeco hiddeco force-pushed the promo-add-freight-ref branch from 7a4d28a to ba3004d Compare April 22, 2024 16:46
@@ -458,6 +457,7 @@ func (r *reconciler) promote(
if err != nil {
return nil, err
}
newStatus = newStatus.WithFreight(&nextFreight)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Personally, I'm not sure I would delegate such a trivial assignment as newStatus.Freight = nextFreight to this new WithFreight() function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the same but figured we could sort it with the other suggestions we would tackle ourselves.

@krancour krancour added this pull request to the merge queue Apr 23, 2024
Merged via the queue into akuity:main with commit cbc96ea Apr 23, 2024
14 of 16 checks passed
@phuongdnguyen phuongdnguyen deleted the promo-add-freight-ref branch April 23, 2024 12:40
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.

add FreightReference to PromotionStatus
3 participants