-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add custom tags to machines #158
base: main
Are you sure you want to change the base?
Conversation
|
internal/service/vmservice/vm.go
Outdated
tags = append(tags, tag) | ||
} | ||
} | ||
if len(tags) > length { |
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.
this check does not account for removal of tags. is this intended?
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, so we don't delete existing tags.
The IP tag i mean.
Which means tags are also immutable as per the machine.
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.
then you need to add this to the webhook or a CRD kubebuilder annotation
@@ -131,3 +144,14 @@ func validateNetworkDeviceMTU(device *infrav1.NetworkDevice) error { | |||
|
|||
return nil | |||
} | |||
|
|||
func validateTags(tags []string) error { | |||
re := regexp.MustCompile(`^[a-zA-Z0-9-_]*$`) |
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 be +
not *
to prevent empty string. Also, have you tried +kubebuilder:validation:Pattern
?
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.
Hmmm,
Let me check
External PRTest runs on external PRs require manual approval. Note: This PR changes the following non-go, non-docs files: |
External PRTest runs on external PRs require manual approval. Note: This PR changes the following non-go, non-docs files: |
|
@@ -427,6 +430,27 @@ func TestReconcileVirtualMachineConfig_ApplyConfig(t *testing.T) { | |||
require.EqualValues(t, task.UPID, *machineScope.ProxmoxMachine.Status.TaskRef) | |||
} | |||
|
|||
func TestReconcileVirtualMachineConfigTags(t *testing.T) { |
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.
Can you add a test case for multiple tags and for zero/empty tags?
This PR add Custom Tags to VM while preserving the current tags.