-
Notifications
You must be signed in to change notification settings - Fork 660
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
Add option to install CRD as a part of chart install in flyte-binary #5967
Add option to install CRD as a part of chart install in flyte-binary #5967
Conversation
Signed-off-by: marrrcin <[email protected]>
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
+1 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5967 +/- ##
==========================================
+ Coverage 36.85% 36.95% +0.09%
==========================================
Files 1310 1310
Lines 131227 131401 +174
==========================================
+ Hits 48362 48555 +193
+ Misses 78668 78632 -36
- Partials 4197 4214 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
…-- PR suggestions Signed-off-by: marrrcin <[email protected]>
ff9ae23
to
892c96e
Compare
@@ -0,0 +1,32 @@ | |||
{{- if not .Values.configuration.propeller.createCRDs }} |
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.
{{- if not .Values.configuration.propeller.createCRDs }} | |
{{- if .Values.configuration.propeller.createCRDs }} |
I think this should be the logic?
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.
No, the logic is correct. If propeller
is set to create CRDs at runtime, then we don't want to install them as a part of helm chart installation.
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.
You're right. Thanks for your patience, this helped me understand better how the CRD install operation works.
@marrrcin to address that |
…-- make helm Signed-off-by: marrrcin <[email protected]>
@davidmirror-ops pushed! :) |
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.
I just installed flyte-binary using this chart and can confirm the expected behavior about the CRD.
Thank you!
Congrats on merging your first pull request! 🎉 |
Awesome, thanks! |
Why are the changes needed?
Due to strict RBAC policies in some k8s clusters, it's impossible to create
ClusterRole
with wide set of permissions for service account used inflyte-binary
deployment. A namespace-scopedRole
could be used, but it requires:Role
andRoleBinding
(possible by creating a sub-chart)FlyteWorkflow
CRD at runtime, after Helm installMore context: https://flyte-org.slack.com/archives/C01P3B761A6/p1729865751682139
What changes were proposed in this pull request?
helm install
How was this patch tested?
3.1. with
helm install
-level CRDs creation enabled and propeller-level CRDs creation disabled.2.2. with
helm install
-level CRDs creation disabled and propeller-level CRDs creation enabled.Setup process
See above.
Check all the applicable boxes
Related PRs
N/A
Docs link
N/A