-
Notifications
You must be signed in to change notification settings - Fork 147
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 49.97% 50.20% +0.22%
==========================================
Files 276 279 +3
Lines 24811 24967 +156
==========================================
+ Hits 12400 12535 +135
- Misses 11724 11740 +16
- Partials 687 692 +5 ☔ 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. |
import { EditAnalysisTemplateModal } from './edit-analysis-template-modal'; | ||
import { EditClusterAnalysisTemplateModal } from './edit-cluster-analysis-template-modal'; | ||
|
||
const ClusterAnalysisTemplatesList = () => { |
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.
Could you please split this component out in separate file? You might want to revert (not git revert) this change back to its previous state anyways for the reason I will put in my next comment.
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.
Sorry for late review, Overall UI LGTM!
Only thing that is blocking is that since Cluster scoped templates are not really project scoped they should not be in individual projects page. I would suggest to have same clickops experience as in project analysis templates but rather on home page (ie. project list page).
Let me know if you have any question
Signed-off-by: BenHesketh21 <[email protected]>
@Marvin9 Thanks for the review. Definitely makes sense to take it out of projects and into a different page. Would we want to add a "Analysis Templates" section to the left side menu? or introduce a similar tab system to the project list page? |
Signed-off-by: BenHesketh21 <[email protected]>
@Marvin9 I've gone with the approach of adding an Analysis button to the left hand menu for now, let me know if you would rather it be done a different way. |
Signed-off-by: BenHesketh21 <[email protected]>
I would suggest, instead, a "Settings" item on the sidebar (with a gear as the icon) and have the UI for this be under a "Verifications" tab on that settings page. We currently lack any sort of UI for these other "cluster-scoped" things:
As we add those, a tabbed settings page would prevent the left sidebar from getting too noisy. And I think this is a fairly conventional approach anyway. |
I'll get on that, thank you for the pointers |
@Marvin9 One thing I'm not so sure on if you could clarify. I'm looking at a vertical tab using the Menu from Antd. I'm struggling to picture how it would look and where it would go? Here's my thinking so far.
Let me know your thoughts, thanks. |
…se analysis templates 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! |
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.