-
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
Changes from 4 commits
4c678f9
73180a2
497c041
502d895
c66c38a
6f368ac
04612ca
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,109 @@ | ||||||
package cmd | ||||||
|
||||||
import ( | ||||||
"fmt" | ||||||
|
||||||
"github.com/kudobuilder/kudo/pkg/kudoctl/cmd/install" | ||||||
"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo" | ||||||
"github.com/pkg/errors" | ||||||
"github.com/spf13/cobra" | ||||||
"github.com/spf13/viper" | ||||||
) | ||||||
|
||||||
var ( | ||||||
updateExample = ` | ||||||
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 | ||||||
|
||||||
# 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
😉 |
||||||
) | ||||||
|
||||||
type updateOptions struct { | ||||||
Namespace string | ||||||
Parameters map[string]string | ||||||
} | ||||||
|
||||||
// defaultOptions initializes the install command options to its defaults | ||||||
var defaultUpdateOptions = &updateOptions{ | ||||||
Namespace: "default", | ||||||
} | ||||||
|
||||||
// newUpgradeCmd creates the install command for the CLI | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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 commentThe 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?
Suggested change
|
||||||
Example: updateExample, | ||||||
RunE: func(cmd *cobra.Command, args []string) error { | ||||||
// Prior to command execution we parse and validate passed parameters | ||||||
var err error | ||||||
options.Parameters, err = install.GetParameterMap(parameters) | ||||||
if err != nil { | ||||||
return errors.WithMessage(err, "could not parse parameters") | ||||||
} | ||||||
return runUpdate(args, options) | ||||||
}, | ||||||
SilenceUsage: true, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. lol good question :D I don't know. Copy-paste 😛
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. okay, should I remove it then from all comands? |
||||||
} | ||||||
|
||||||
upgradeCmd.Flags().StringArrayVarP(¶meters, "parameter", "p", nil, "The parameter name and value separated by '='") | ||||||
upgradeCmd.Flags().StringVar(&options.Namespace, "namespace", defaultOptions.Namespace, "The namespace where the instance you want to upgrade is installed in.") | ||||||
|
||||||
const usageFmt = "Usage:\n %s\n\nFlags:\n%s" | ||||||
upgradeCmd.SetUsageFunc(func(cmd *cobra.Command) error { | ||||||
fmt.Fprintf(upgradeCmd.OutOrStderr(), usageFmt, upgradeCmd.UseLine(), upgradeCmd.Flags().FlagUsages()) | ||||||
return nil | ||||||
}) | ||||||
return upgradeCmd | ||||||
} | ||||||
|
||||||
func validateUpdateCmd(args []string, options *updateOptions) error { | ||||||
if len(args) != 1 { | ||||||
return errors.New("expecting exactly one argument - name of the instance installed in your cluster") | ||||||
} | ||||||
if len(options.Parameters) == 0 { | ||||||
return errors.New("Need to specify at least one parameter to override via -p otherwise there is nothing to update") | ||||||
} | ||||||
|
||||||
return nil | ||||||
} | ||||||
|
||||||
func runUpdate(args []string, options *updateOptions) error { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. ...for now 😉 But I'll leave this up to you |
||||||
err := validateUpdateCmd(args, options) | ||||||
if err != nil { | ||||||
return err | ||||||
} | ||||||
instanceToUpdate := args[0] | ||||||
|
||||||
kc, err := kudo.NewClient(options.Namespace, viper.GetString("kubeconfig")) | ||||||
if err != nil { | ||||||
return errors.Wrap(err, "creating kudo client") | ||||||
} | ||||||
|
||||||
return update(instanceToUpdate, kc, options) | ||||||
} | ||||||
|
||||||
func update(instanceToUpdate string, kc *kudo.Client, options *updateOptions) error { | ||||||
// Make sure the instance you want to upgrade exists | ||||||
instance, err := kc.GetInstance(instanceToUpdate, options.Namespace) | ||||||
if err != nil { | ||||||
return errors.Wrapf(err, "verifying the instance does not already exist") | ||||||
} | ||||||
if instance == nil { | ||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this comment makes sense here |
||||||
err = kc.UpdateInstance(instanceToUpdate, options.Namespace, nil, options.Parameters) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, why do we pass There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
if err != nil { | ||||||
return errors.Wrapf(err, "updating instance %s", instanceToUpdate) | ||||||
} | ||||||
fmt.Printf("Instance %s was updated ヽ(•‿•)ノ") | ||||||
return nil | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
package cmd | ||
|
||
import ( | ||
"strings" | ||
"testing" | ||
|
||
"github.com/kudobuilder/kudo/pkg/apis/kudo/v1alpha1" | ||
util "github.com/kudobuilder/kudo/pkg/util/kudo" | ||
v1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
) | ||
|
||
func TestUpdateCommand_Validation(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
args []string | ||
parameters map[string]string | ||
err string | ||
}{ | ||
{"no argument", []string{}, map[string]string{"param": "value"}, "expecting exactly one argument - name of the instance installed in your cluster"}, | ||
{"too many arguments", []string{"aaa", "bbb"}, map[string]string{"param": "value"}, "expecting exactly one argument - name of the instance installed in your cluster"}, | ||
{"no instance name", []string{"arg"}, map[string]string{}, "Need to specify at least one parameter to override via -p otherwise there is nothing to update"}, | ||
} | ||
|
||
for _, tt := range tests { | ||
cmd := newUpdateCmd() | ||
cmd.SetArgs(tt.args) | ||
for _, v := range tt.parameters { | ||
cmd.Flags().Set("p", v) | ||
} | ||
_, err := cmd.ExecuteC() | ||
if err.Error() != tt.err { | ||
t.Errorf("%s: expecting error %s got %v", tt.name, tt.err, err) | ||
} | ||
} | ||
} | ||
|
||
func TestUpdate(t *testing.T) { | ||
testInstance := v1alpha1.Instance{ | ||
TypeMeta: metav1.TypeMeta{ | ||
APIVersion: "kudo.dev/v1alpha1", | ||
Kind: "Instance", | ||
}, | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Labels: map[string]string{ | ||
"controller-tools.k8s.io": "1.0", | ||
util.OperatorLabel: "test", | ||
}, | ||
Name: "test", | ||
}, | ||
Spec: v1alpha1.InstanceSpec{ | ||
OperatorVersion: v1.ObjectReference{ | ||
Name: "test-1.0", | ||
}, | ||
}, | ||
} | ||
|
||
installNamespace := "default" | ||
tests := []struct { | ||
name string | ||
instanceExists bool | ||
parameters map[string]string | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Nice tests 👏 How about giving the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. okay I did it in here |
||
} | ||
|
||
for _, tt := range tests { | ||
c := newTestClient() | ||
if tt.instanceExists { | ||
c.InstallInstanceObjToCluster(&testInstance, installNamespace) | ||
} | ||
|
||
err := update(testInstance.Name, c, &updateOptions{ | ||
Namespace: installNamespace, | ||
Parameters: tt.parameters, | ||
}) | ||
if err != nil { | ||
if !strings.Contains(err.Error(), tt.errMessageContains) { | ||
t.Errorf("%s: expected error '%s' but got '%v'", tt.name, tt.errMessageContains, err) | ||
} | ||
} else if tt.errMessageContains != "" { | ||
t.Errorf("%s: expected no error but got %v", tt.name, err) | ||
} else { | ||
// the upgrade should have passed without error | ||
instance, err := c.GetInstance(testInstance.Name, installNamespace) | ||
if err != nil { | ||
t.Errorf("%s: error when getting instance to verify the test: %v", tt.name, err) | ||
} | ||
for k, v := range tt.parameters { | ||
value, ok := instance.Spec.Parameters[k] | ||
if !ok || value != v { | ||
t.Errorf("%s: expected parameter %s to be updated to %s but params are %v", tt.name, k, v, instance.Spec.Parameters) | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import ( | |
"github.com/kudobuilder/kudo/pkg/kudoctl/cmd/install" | ||
"github.com/kudobuilder/kudo/pkg/kudoctl/util/kudo" | ||
"github.com/kudobuilder/kudo/pkg/kudoctl/util/repo" | ||
util "github.com/kudobuilder/kudo/pkg/util/kudo" | ||
"github.com/pkg/errors" | ||
"github.com/spf13/cobra" | ||
"github.com/spf13/viper" | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Hu? Why did this change from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch :) |
||
if err != nil { | ||
return errors.Wrapf(err, "updating instance to point to new operatorversion %s", newOv.Name) | ||
} | ||
|
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