virtctl: Apply --namespace to created manifests #10167
Conversation
With this change virtctl applies the namespace passed with the --namespace flag to the created VirtualMachine manifest. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
With this change virtctl applies the namespace passed with the --namespace flag to the created VirtualMachinePreference manifest. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
With this change virtctl applies the namespace passed with the --namespace flag to the created VirtualMachineInstancetype manifest. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
With this change virtctl applies the namespace passed with the --namespace flag to the created VirtualMachineClone manifest. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>
kubevirt-bot
added
release-note
|
/area instancetype |
|
/cc @lyarwood |
|
/cc @opokornyy |
0xFelix
changed the title
|
/retest-required |
|
/cc @alicefr Could you take a look please? :) |
| } | ||
| if overridden { | ||
| c.namespace = namespace | ||
| c.namespaced = true |
There was a problem hiding this comment.
Do we still need the namespaced flag? We could by default create VirtualMachineClusterInstancetype and based on namespace flag decide if we want the VirtualMachineInstancetype instead. What do you think about that?
There was a problem hiding this comment.
I'd keep the namespaced flag so it is still possible to create a VirtualMachineInstancetype that does not have the namespace set.
| } | ||
| if overridden { | ||
| c.namespace = namespace | ||
| c.namespaced = true |
There was a problem hiding this comment.
Same question like for VirtualMachineInstancetype.
| @@ -23,14 +23,15 @@ import ( | |||
| "fmt" | |||
| "strings" | |||
|
|
|||
| "kubevirt.io/kubevirt/pkg/pointer" | |||
|
|
|||
| "github.com/spf13/cobra" | |||
There was a problem hiding this comment.
it's not related with this PR but, newline?
There was a problem hiding this comment.
I moved this local import to the bottom of the imports on purpose.
There was a problem hiding this comment.
Sorry, I didn't explain myself properly. I meant between:
"github.com/spf13/cobra"v1
"k8s.io/api/core/v1"There was a problem hiding this comment.
Those are all remote imports, so AFAIK we don't separate them further.
| @@ -21,9 +21,9 @@ package create | |||
|
|
|||
| import ( | |||
| "github.com/spf13/cobra" | |||
There was a problem hiding this comment.
Basically the same as the other comment: First stdlib imports, then remote packages, then local packages. I did this on purpose while touching the imports anyways.
|
@alicefr Ping :) |
|
It looks good, the only super nit I have, if we should put the common field in a struct instead of adding them to every types. Like: type createCommon {
name string
namespace string
clientConfig clientcmd.ClientConfig
}(not sure about the name) and a common function to set the defaults for those values. |
|
/approve |
kubevirt-bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alicefr 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 |
kubevirt-bot
added
the
approved
|
I have looked at your suggestions and I don't think they necessarily make things any easier. It would require public fields, which I'd like to avoid and a common defaults setter for the namespace is not possible, because they all require slightly different logic. Maybe in a follow up? |
That's ok, thanks for your work! |
kubevirt-bot
removed
the
do-not-merge/hold
|
/retest-required |
What this PR does / why we need it:
With this PR virtctl applies the namespace passed with the
--namespace flag to the following created manifests:
Release note: