-
Notifications
You must be signed in to change notification settings - Fork 228
Conversation
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.
Thank you for submitting this PR, @darkowlzz :-)
Not sure if you think my comment to change Split
to SplitN
is making sense?
Other than this small nit, it LGTM
pkg/metadata/metadata.go
Outdated
} | ||
|
||
for _, label := range labels { | ||
kv := strings.Split(label, "=") |
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 use SplitN(label, "=", 2)
, wdyt?
if so, the value could also be possible to contain other = chars too.
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, that's a good idea. I checked what happens if I pass "k1=v1,k2=v2"
as label to docker, and it splits at first separator and accepts the rest of the string as the value.
"Labels": {
"k1": "v1,k2=v2"
}
Updating the test to not fail for this case.
Thanks!
Adds a new flag --label to VM create command for setting label on the VM object. The flag is of StringArray type and can be used multiple times to set more than one label. ``` $ ignite create weaveworks/ignite-ubuntu --name vm -l k1=v1 -l k2=v2 ```
Added another check to ensure that the label name isn't empty. Updated the test with this case. |
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.
LGTM
Thank you @darkowlzz for this wonderful PR!! |
Adds a new flag
--label
to VM create command for setting label on the VMobject. The flag is of StringArray type and can be used multiple times
to set more than one label.
vm inspect result: