feat(api): add GetAnalysisRun endpoint #1682
Conversation
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
✅ Deploy Preview for docs-kargo-akuity-io canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1682 +/- ##
==========================================
- Coverage 44.10% 43.93% -0.17%
==========================================
Files 202 203 +1
Lines 13043 13061 +18
==========================================
- Hits 5753 5739 -14
- Misses 7045 7075 +30
- Partials 245 247 +2 ☔ View full report in Codecov by Sentry. |
hiddeco
force-pushed
the
api-get-analysisrun
branch
2 times, most recently
from
89d5330 to
c483514
Compare
hiddeco
force-pushed
the
api-get-analysisrun
branch
2 times, most recently
from
9542f39 to
42eb718
Compare
| @@ -60,4 +60,10 @@ data: | |||
| ARGOCD_URLS: {{ range $key, $val := .Values.api.argocd.urls }}{{ $key }}={{ $val }},{{- end }} | |||
| {{- end }} | |||
| ROLLOUTS_INTEGRATION_ENABLED: {{ quote .Values.api.rollouts.integrationEnabled }} | |||
| {{- if .Values.controller.rollouts.integrationEnabled }} | |||
There was a problem hiding this comment.
It bugs me that we're using a .Values.controller... setting for an API server concern...
I suspect we may eventually remove support for two separate clients for Kargo control plane and Rollouts control plane, so I wouldn't waste time fixing this. Just calling out that I recognize the peculiarity of this and think we can let it slide for now.
There was a problem hiding this comment.
Actually, I take that back. It's easy enough to fix this. Sorry for the flip-flopping.
There was a problem hiding this comment.
Ah, that's an actual copy/paste mistake from my side. The thing that's harder to change though is the .controller.rollouts.analysisRunsNamespace value.
There was a problem hiding this comment.
Also, we'll still need this flag even if we remove support for the second client, so ya... any initial thought I had that this was temporary was completely off.
There was a problem hiding this comment.
Is there a scenario in which API and controller are configured differently? If not, we could move .rollouts up under .Values itself, with fallback to the previous fields where applicable.
There was a problem hiding this comment.
Addressed the remaining .controller reference by amending c885b16
There was a problem hiding this comment.
Is there a scenario in which API and controller are configured differently?
In a single Helm release... no.
API server is only ever installed on the control plane. Controllers can be more distributed, but any API server and controller that are co-located would probably agree on what integrations are enabled.
I agree we could move it up, although it is a breaking change... 🤔
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
Signed-off-by: Hidde Beydals <hidde@hhh.computer>
hiddeco
force-pushed
the
api-get-analysisrun
branch
from
42eb718 to
8c055fa
Compare
| @@ -136,12 +144,14 @@ func NewServer( | |||
| cfg config.ServerConfig, | |||
| kubeClient kubernetes.Client, | |||
| internalClient client.Client, | |||
| rolloutsEnabled bool, | |||
There was a problem hiding this comment.
I suspect longer term we will find more things we need to pass in and might opt for some kind of config object here, but I think this is suitable for now.
There was a problem hiding this comment.
Yes, or ServerConfig being utilized better. As I am not sure what to think of passing config and options.
There was a problem hiding this comment.
🤦♂️ I overlooked the ServerConfig that was already there... I might prefer if this bool were a field in there instead of loose.
There was a problem hiding this comment.
Ah... you merged, but this was non-blocking.
There was a problem hiding this comment.
I'll do a quick follow up tomorrow as I initially didn't want to touch it due to it dealing with loading user defined settings.
But I think a way I do feel comfortable with is to overwrite / downgrade these settings if they are not feasible (due to e.g. the CRDs not being present).
There was a problem hiding this comment.
It's probably possible though for the CRDs to be present on the cluster and the integration is still not desired.
This adds an endpoint to the API to get an AnalysisRun, to allow the UI to display the full YAML, besides offering a list based on the verification history information from a Stage.
The client used to get the AnalysisRun is set up in the same way as for the controller, and isolated from the "Kargo client" used for other resource kinds.