feat:wasm deploy support istiodDeployName arg in command #248
Conversation
|
Waiting for approval from someone in the solo-io org to start testing. |
|
/cc @Sodman |
There was a problem hiding this comment.
Thanks for opening a PR for this!
The new flag code is causing a runtime error, StringVarP needs to be updated to StringVar instead.
You'll also need to include a changelog for this feature so that it's documented in our release notes. An example changelog you can use is https://github.com/solo-io/wasm/blob/master/tools/wasme/changelog/v0.0.33/patch-context.yaml. For this change please include a link to the GitHub Issue, the name of the new flag, and the default value. Thanks!
|
|
||
| puller pull.ImagePuller // set by load | ||
| } | ||
|
|
||
| func (opts *istioOpts) addToFlags(flags *pflag.FlagSet) { | ||
| flags.StringToStringVarP(&opts.workload.Labels, "labels", "l", nil, "labels of the deployment or daemonset into which to inject the filter. if not set, will apply to all workloads in the target namespace") | ||
| flags.StringVarP(&opts.workload.Namespace, "namespace", "n", "default", "namespace of the workload(s) to inject the filter.") | ||
| flags.StringVarP(&opts.istiodDeploymentName, "istiod-name", "istiod", "istiod", "deployment name of the istiod") |
There was a problem hiding this comment.
The StringVarP variant is for when we have a 1-letter shortcut for the flag (eg "namespace" / "n"). Since there's no shorthand we can just use the regular StringVar here.
| flags.StringVarP(&opts.istiodDeploymentName, "istiod-name", "istiod", "istiod", "deployment name of the istiod") | |
| flags.StringVar(&opts.istiodDeploymentName, "istiod-name", "istiod", "deployment name of the istiod") |
(This is the reason the tests are currently failing, it causes a runtime error).
| @@ -105,25 +109,26 @@ type Provider struct { | |||
| WaitForCacheTimeout time.Duration | |||
| } | |||
|
|
|||
| func NewProvider(ctx context.Context, kubeClient kubernetes.Interface, client ezkube.Ensurer, puller pull.ImagePuller, workload Workload, cache Cache, parentObject ezkube.Object, onWorkload func(workloadMeta metav1.ObjectMeta, err error), istioNamespace string, cacheTimeout time.Duration, ignoreVersionCheck bool) (*Provider, error) { | |||
| func NewProvider(ctx context.Context, kubeClient kubernetes.Interface, client ezkube.Ensurer, puller pull.ImagePuller, workload Workload, cache Cache, parentObject ezkube.Object, onWorkload func(workloadMeta metav1.ObjectMeta, err error), istioNamespace, IstiodDeploymentName string, cacheTimeout time.Duration, ignoreVersionCheck bool) (*Provider, error) { | |||
There was a problem hiding this comment.
Small nit: can we lowercase IstiodDeploymentName since it's local in scope here?
| func NewProvider(ctx context.Context, kubeClient kubernetes.Interface, client ezkube.Ensurer, puller pull.ImagePuller, workload Workload, cache Cache, parentObject ezkube.Object, onWorkload func(workloadMeta metav1.ObjectMeta, err error), istioNamespace, IstiodDeploymentName string, cacheTimeout time.Duration, ignoreVersionCheck bool) (*Provider, error) { | |
| func NewProvider(ctx context.Context, kubeClient kubernetes.Interface, client ezkube.Ensurer, puller pull.ImagePuller, workload Workload, cache Cache, parentObject ezkube.Object, onWorkload func(workloadMeta metav1.ObjectMeta, err error), istioNamespace, istiodDeploymentName string, cacheTimeout time.Duration, ignoreVersionCheck bool) (*Provider, error) { |
| ParentObject: parentObject, | ||
| OnWorkload: onWorkload, | ||
| IstioNamespace: istioNamespace, | ||
| IstiodDeploymentName: IstiodDeploymentName, |
There was a problem hiding this comment.
Same here
| IstiodDeploymentName: IstiodDeploymentName, | |
| IstiodDeploymentName: istiodDeploymentName, |
Description
Please include a summary of the changes.
This bug fixes ... \ This new feature can be used to ...
Context
ref #245
Users ran into this bug doing ... \ Users needed this feature to ...
Checklist:
make install-deps generated-codefromtools/wasme/clito ensure there will be no code diff