-
Notifications
You must be signed in to change notification settings - Fork 176
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: Adding ClusterAnalysisTemplate support for Stage verifications #2673
base: main
Are you sure you want to change the base?
feat: Adding ClusterAnalysisTemplate support for Stage verifications #2673
Conversation
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2673 +/- ##
==========================================
+ Coverage 52.47% 52.59% +0.11%
==========================================
Files 292 295 +3
Lines 26733 26868 +135
==========================================
+ Hits 14029 14132 +103
- Misses 11939 11965 +26
- Partials 765 771 +6 ☔ View full report in Codecov by Sentry. |
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
@BenHesketh21 thank you for your patience with this. I've finally started reviewing this in earnest. What I've looked at so far looks solid, but this is big and there's a lot more to still look at. Due to the size and complexity, I'm going to ask @hiddeco and @Marvin9 for help with this. Group effort. @hiddeco since this touches the Stages reconciler, which you are actively refactoring, controller changes here probably need to be on your radar. @Marvin9 if you don't mind looking at the UI portions of this, it would be a huge help. I am happy to review the remainder -- e.g. API server changes, docs, and such. We will try to get this into v1.1.0, but it cannot be guaranteed. |
…se analysis templates Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
Sure thing @BenHesketh21 here is how I imagine it would look like considering more settings incoming Let me know if its feasible to you otherwise I would refactor the layout later this PR and for now we can keep the tab as in your screenshot. I will put my comments after we finalise what are we scoping for this PR. Thanks! |
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
@Marvin9 Apologies for the delay, I had to re-do a lot of the controller work due to the refactor, I have moved the tab to be vertical. Let me know what you think. |
Signed-off-by: BenHesketh21 <[email protected]>
Signed-off-by: BenHesketh21 <[email protected]>
We're trying to wrap up v1.2 work by 12/20 with plans to release after the holidays. It might be too ambitious for us to get this merged before then. I'm tentatively moving this to the v1.3 milestone, but if it somehow becomes possible to sneak this into v1.2, I won't hesitate. |
Sorry @BenHesketh21 do you mind if I change the design a bit? just adjusting color, spacing and layout.. Other than that I don't think there is anything that should block UI |
@Marvin9 Please be my guest. UI isn't my strongest area. |
Signed-off-by: Mayursinh Sarvaiya <[email protected]>
Signed-off-by: Mayursinh Sarvaiya <[email protected]>
Signed-off-by: Mayursinh Sarvaiya <[email protected]>
Done with the UI and it LGTM! Thanks @BenHesketh21 @krancour it is now ready to review the rest of the things.. |
Signed-off-by: BenHesketh21 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation-wise, this all looks pretty straight-forward and is a job well done.
I just have a minor suggestion in how a ClusterAnalysisTemplate
is referenced, please see my comment.
Signed-off-by: Ben Hesketh <[email protected]>
b469674
to
72e799a
Compare
Tested this end-to-end, and everything is working as expected. Great work @BenHesketh21, and you should be done here. There appears to be a small issue with the UI however, @Marvin9: |
closes #2481
ClusterAnalysisTemplate Support
This PR adds the ability to refer to cluster level analysis templates using the
clusterScope
option (inspired by Argo Rollouts).The verification process finds all the templates and combines them in one run, in the same way as before but now will also find referenced ClusterAnalysisTemplates if
clusterScope
is set to true.The UI has also been updated to show both scopes of analysis templates.