feat: add argocd application info to promotion event

[hanxiaop]

This PR adds the applications updated during promotion to the event, so we can get the related application info. Also, I’ve moved NewPromotionEventAnnotations from the api directory to internal/api since it’s only used in internal packages, to avoid cyclic imports.

The sample results will be something like:

...
 metadata:
    annotations:
      event.kargo.akuity.io/actor: admin
      event.kargo.akuity.io/applications: '[{"name":"kargo-demo-test"}]'
      event.kargo.akuity.io/freight-alias: billowing-shark
...

[netlify]

✅ Deploy Preview for docs-kargo-io ready!

Name	Link
🔨 Latest commit	a9e29fb
🔍 Latest deploy log	https://app.netlify.com/sites/docs-kargo-io/deploys/6720d95fa1d5cc00083e6b41
😎 Deploy Preview	https://deploy-preview-2789.docs.kargo.io
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[gdsoumya]

any specific reason to move this code from the v1alpha1/event?

[hanxiaop]

There are two main reasons:

1. The function is only used in internal packages.
2. If I keep it in the original directory, importing ArgoCDUpdateConfig from the internal package will cause import cycle not allowed, as the v1alpha1/ package is also imported in the internal package.

[krancour]

I understand the import cycle you're trying to avoid, but I don't think this package is a good home for this. (idk if its old home was any better, tbh.)

"API" is overloaded. I wish this package had been named apiserver instead, as this package is all API server endpoints, interceptors, etc. and this functionality you're moving to here is used much more broadly -- by the controller, by the webhooks, etc.

Is it possible this might be better off living in the existing event package (e.g. Live alongside things like func NewRecorder())?

[hanxiaop]

@krancour I tried placing it in the existing event package, but realized it is better for k8s event type. Since I couldn’t find a better location, I’ve refactored some files into internal folders in #2808. PTAL if this makes sense. Thank you!

[codecov]

Codecov Report

Attention: Patch coverage is 72.85714% with 19 lines in your changes missing coverage. Please review.

Project coverage is 50.16%. Comparing base (cf12780) to head (a9e29fb).
Report is 34 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/event/promotion.go	74.24%	7 Missing and 10 partials ⚠️
internal/controller/promotions/promotions.go	0.00%	1 Missing ⚠️
internal/controller/stages/stages.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2789      +/-   ##
==========================================
+ Coverage   48.83%   50.16%   +1.33%     
==========================================
  Files         270      275       +5     
  Lines       23932    24410     +478     
==========================================
+ Hits        11687    12246     +559     
+ Misses      11616    11504     -112     
- Partials      629      660      +31     

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

[krancour]

We're trying really hard to make built-in directives "not special." Ideally, the rest of Kargo would have as little awareness of their internals as it will eventually have of the internals of third-party directives.

That being said, I understand this might seem like the only practical way to get the information you're trying to include in the events. This, unfortunately, wouldn't be the first spot where we compromised on this ideal because it seemed like the only way to get something done.

Ultimately, I think this ties into a much broader question of how coupled do we want to be or not want to be to Argo CD. Implementation details aside, just including this information in the Promotion event to begin with says something about Kargo's relationship to Argo CD being special in some way. I think this may be a question for @jessesuen.

[hanxiaop]

We're trying really hard to make built-in directives "not special." Ideally, the rest of Kargo would have as little awareness of their internals as it will eventually have of the internals of third-party directives.

The intention of maintaining flexibility and avoiding any special logics for built-in and third-party directives makes sense to me. However I'm still a bit unclear about the design here. From my perspective, this little awareness is from code level instead of user level. Users still see the results the directives processed, but developers are limited to access the output of runners they have written.

Ultimately, I think this ties into a much broader question of how coupled do we want to be or not want to be to Argo CD. Implementation details aside, just including this information in the Promotion event to begin with says something about Kargo's relationship to Argo CD being special in some way.

I would also like to know the idea behind #2545. As a Kargo newbie, when I saw the argocd-update step inPromotion resource, I would expect there are some ArgoCD integrations with Kargo. Otherwise it seems we might not need this directive from the lower-level implementations. Seems like this can also be related to how we can integrate with projects that manage workloads?

[hiddeco]

I feel this new location is making things worse, as it now mixes simple Kubernetes implementations with our API-specific code. IMHO, the better option is/was internal/event. But before you make this change @hanxiaop, I would like @krancour to agree.

[hanxiaop]

@hiddeco I tried to move the entire api/v1alpha1/event.go file and kubernetes events related files to internal/event in #2808. However as you can see, this would require moving multiple files since some helpers rely on functions within that file. Moving just one function to internal/event doesn’t seem to make much sense. I’d also like to wait for consensus on the best location for this function.

[hiddeco]

Please move this to internal/event/promotion.go.

[hiddeco]

Plus maybe rename it to NewPromotionAnnotations, as event already should be enough to indicate it's about events.

[hiddeco]

Additional candidate to be moved from the public API package is FormatEventUserActor (-> FormatUserActor).

[hanxiaop]

@hiddeco Thank you for the suggestion! I've moved this function to internal/event/promotion.go.

Additional candidate to be moved from the public API package is FormatEventUserActor (-> FormatUserActor).

I've tried this but it still caused the import cycle issue like:

0.602   imports github.com/akuity/kargo/api/v1alpha1
0.602   imports github.com/akuity/kargo/internal/event
0.602   imports github.com/akuity/kargo/api/v1alpha1: import cycle not allowed

I think we could only move NewPromotionAnnotations here for minimal changes. We can discuss further improvements if necessary and I'd be very happy to contribute more.

[hiddeco]

That's fair.