-
Notifications
You must be signed in to change notification settings - Fork 103
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
Update command #673
Update command #673
Conversation
} | ||
return runUpdate(args, options) | ||
}, | ||
SilenceUsage: 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.
Why are we setting this?
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.
lol good question :D I don't know. Copy-paste 😛
SilenceUsage is an option to silence usage when an error occurs.
... sounds reasonable to me though 🤷♀ lot of errors can come up from non-usage errors so maybe that's why we have decided to not print it there? No hard opinion though...
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.
That seems like a debug remnant? Not sure why we would set 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.
okay, should I remove it then from all comands?
pkg/kudoctl/cmd/update.go
Outdated
The update argument must be a name of the instance. | ||
|
||
# Update dev-flink instance with setting parameter param with value value | ||
kubectl kudo upgrade dev-flink -p param=value |
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.
Lol
kubectl kudo upgrade dev-flink -p param=value | |
kubectl kudo update dev-flink -p param=value |
pkg/kudoctl/cmd/update.go
Outdated
kubectl kudo upgrade dev-flink -p param=value | ||
|
||
# Update dev-flink instance in namespace services with setting parameter param with value value | ||
kubectl kudo upgrade dev-flink -n services -p param=value` |
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.
kubectl kudo upgrade dev-flink -n services -p param=value` | |
kubectl kudo update dev-flink -n services -p param=value` |
😉
} | ||
return runUpdate(args, options) | ||
}, | ||
SilenceUsage: 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.
That seems like a debug remnant? Not sure why we would set it.
pkg/kudoctl/cmd/update.go
Outdated
Namespace: "default", | ||
} | ||
|
||
// newUpgradeCmd creates the install command for the CLI |
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.
// newUpgradeCmd creates the install command for the CLI | |
// newUpdateCmd creates the install command for the CLI |
pkg/kudoctl/cmd/update.go
Outdated
func newUpdateCmd() *cobra.Command { | ||
options := defaultUpdateOptions | ||
var parameters []string | ||
upgradeCmd := &cobra.Command{ |
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.
upgradeCmd := &cobra.Command{ | |
updateCmd := &cobra.Command{ |
return nil | ||
} | ||
|
||
func runUpdate(args []string, options *updateOptions) error { |
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 have this patter (which I like) where we separate building of the command from the actual code that is executed in different files. It might seem like overkill here, but these things tend to grow 😉
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.
yeah my current personal opinion is to keep it in one file as long as the command is simple enough...
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.
...for now 😉 But I'll leave this up to you
pkg/kudoctl/cmd/upgrade.go
Outdated
@@ -157,7 +158,7 @@ func upgrade(newOv *v1alpha1.OperatorVersion, kc *kudo.Client, options *options) | |||
} | |||
|
|||
// Change instance to point to the new OV and optionally update parameters | |||
err = kc.UpdateInstance(options.InstanceName, options.Namespace, newOv.Name, options.Parameters) | |||
err = kc.UpdateInstance(options.InstanceName, options.Namespace, util.String(ov.Name), options.Parameters) |
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.
Hu? Why did this change from newOv
to ov
?
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.
good catch :)
pkg/kudoctl/cmd/update.go
Outdated
return fmt.Errorf("instance %s in namespace %s does not exist in the cluster", instanceToUpdate, options.Namespace) | ||
} | ||
|
||
// Change instance to point to the new OV and optionally update parameters |
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 don't think this comment makes sense here
} | ||
|
||
// Change instance to point to the new OV and optionally update parameters | ||
err = kc.UpdateInstance(instanceToUpdate, options.Namespace, nil, options.Parameters) |
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.
also, why do we pass nil
as operatorVersionName
here?
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.
because we don't want to update the version in this call. I use nil here as I would use None
in scala
errMessageContains string | ||
}{ | ||
{"instance does not exist", false, map[string]string{"param": "value"}, "instance test in namespace default does not exist in the cluster"}, | ||
{"update parameters", true, map[string]string{"param": "value"}, ""}, |
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 tests 👏 How about giving the testInstance
an existing parameter param: oldValue
?
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.
yeah I think the current patch implementation is incorrect, since this code for patch is already on master, I'll address that in another PR and add the test as well
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.
okay I did it in here
okay, the patch for parameters was not working 100% because if you had parameters in place, it dropped them and replaced with new, which is not correct. I switched to merge patch and now it seems to be OK (thx @zen-dog )
|
updateCmd := &cobra.Command{ | ||
Use: "update <instance-name>", | ||
Short: "Update installed KUDO operator.", | ||
Long: `Update installed KUDO operator with new parameters.`, |
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 also mention explicitly that one can not remove existing parameters this way?
Long: `Update installed KUDO operator with new parameters.`, | |
Long: `Update installed KUDO operator with new parameters. Existing parameters can not be removed for now.`, |
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 you promise to add an IT ;)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alenkacz, jbarrick-mesosphere, zen-dog 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 |
What type of PR is this?
/component kudoctl
/kind enhancement
What this PR does / why we need it:
kudo update
patches running instance with new parameter values.Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: