-
Notifications
You must be signed in to change notification settings - Fork 29
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
✨Added support for helm options like wait, skip-crds, timeout, waitForJobs, etc. #82
Conversation
Hi @manoj-nutanix. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
||
// If 'Wait' is set, we need to set default 'Timeout' to make install successful. | ||
if p.Spec.Options != nil && p.Spec.Options.Wait { | ||
p.Spec.Options.Timeout = &metaV1.Duration{Duration: time.Second * 600} |
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.
Should we make this timeout a user-configurable flag so that when they enable wait they can also configure the upper time limit?
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.
Yes. My bad, forgot to add p.Spec.Options.Timeout == nil
check. Thanks for the pointer.
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.
Updated. Please review.
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.
let's make this default timeout value a const so that we can easily write a UT and validate the default flow from a single source of truth
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.
We could also check in the Helm repo too to see if there's a const they already set.
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.
Sure, updated. Please review.
This looks really cool, thanks @manoj-nutanix! |
Thanks, could you please comment |
/ok-to-test |
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.
Nice work! I had a couple points to clarify but looks good overall. Have you tried testing it out yet?
|
||
// If 'Wait' is set, we need to set default 'Timeout' to make install successful. | ||
if p.Spec.Options != nil && p.Spec.Options.Wait { | ||
p.Spec.Options.Timeout = &metaV1.Duration{Duration: time.Second * 600} |
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.
We could also check in the Helm repo too to see if there's a const they already set.
@@ -38,13 +43,29 @@ func (r *HelmChartProxy) SetupWebhookWithManager(mgr ctrl.Manager) error { | |||
|
|||
var _ webhook.Defaulter = &HelmChartProxy{} | |||
|
|||
var trueValuePtr = func(i bool) *bool { return &i }(true) |
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.
We can use the pointer package to get the true value pointer instead.
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.
Sure, updated. Please review.
api/v1alpha1/helmchartproxy_types.go
Outdated
@@ -58,6 +58,104 @@ type HelmChartProxySpec struct { | |||
// fields from each selected workload Cluster and programatically create and set values. | |||
// +optional | |||
ValuesTemplate string `json:"valuesTemplate,omitempty"` | |||
|
|||
// Options represents the helm setting options which can be used to control behaviour of helm operations(Install, Upgrade, Delete, etc) |
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.
// Options represents the helm setting options which can be used to control behaviour of helm operations(Install, Upgrade, Delete, etc) | |
// Options represents the CLI flags passed to Helm operations (i.e. install, upgrade, delete) and include options such as wait, skipCRDs, timeout, waitForJobs, etc. |
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.
Sure, updated. Please review.
|
||
// SkipCRDs controls whether CRDs should be installed during install/upgrade operation. | ||
// If set, no CRDs will be installed. | ||
// By default, CRDs are installed if not already present. |
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.
Nit: maybe move this back to the line above.
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.
Sure, updated. Please review.
Yes, I've done sanity testing for all operation viz. helm install, upgrade, uninstall. Do we have automation tests written somewhere? I could not find any test files in this repo.
I could not find any const for timeout so I've declared our const. FYI, helm cli uses 300s timeout but I've kept it on higher side (600s) for safety due to reconciliation pattern as user wouldn't want to keep checking failure and update HCP CRs on every failure. |
@Jont828 @jackfrancis I've addressed review comments and added support of helm flags for Uninstall operation as well, please review. |
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.
Think we're almost good!
@@ -38,13 +44,31 @@ func (r *HelmChartProxy) SetupWebhookWithManager(mgr ctrl.Manager) error { | |||
|
|||
var _ webhook.Defaulter = &HelmChartProxy{} | |||
|
|||
var trueValuePtr = pointer.Bool(true) |
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.
Nit: I think the convention we've been using has just been to use the function return value instead of declaring a variable for it
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.
sure, updated. Please review.
Gotcha. I haven't set up e2e tests or controller tests yet, but that's definitely going to be a bigger priority moving forward. I'd be happy to collab on that if you'd like as well. |
@manoj-nutanix LGTM pending squash |
Just to understand, why don't we use |
…Jobs, etc. Added nil check for Timeout in webhook defaulting to make Timeout user configurable Added const helmTimeoutSeconds in HCP webhook checks Added support for helm flags for uninstall operation as well Removed var trueValuePtr from webhook check
@manoj-nutanix That's a good question, it's just the convention we've been following with CAPI PRs. I think the main reason is that we want to use the approve command to let the K8s bot merge it so that there's your commit and the merge commit from prow. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Jont828, manoj-nutanix The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I believe, this label will do the job for us. |
What this PR does / why we need it:
PR adds support for helm options like wait, skip-crds, timeout, waitForJobs, etc. with help of CRD fields. Users can now control behaviour of helm operations(Install, Upgrade, Delete, etc) with helm options by specifying it in HCP CR as follows -
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #79