Fix custom resource build prefix #218
Conversation
|
|
There was a problem hiding this comment.
Just a few comments on that, but overall the code looks good to me, once addressed we can merge that.
Thanks for the contribution @Swampen, much appreciated
builder/azure/arm/config.go
Outdated
| @@ -1265,8 +1264,8 @@ func assertManagedImageDataDiskSnapshotName(name, setting string) (bool, error) | |||
| } | |||
|
|
|||
| func assertResourceNamePrefix(name, setting string) (bool, error) { | |||
| if !isValidAzureName(reResourceNamePrefix, name) { | |||
| return false, fmt.Errorf("The setting %s must only contain characters from a-z, A-Z, 0-9 and _ and the maximum length is 10 characters", setting) | |||
| if !reResourceNamePrefix.Match([]byte(name)) { | |||
There was a problem hiding this comment.
Alternatively here you could use MatchString, that wouldn't require converting the name to a byte array
There was a problem hiding this comment.
Nice catch :) Changed
builder/azure/arm/config.go
Outdated
| @@ -65,7 +64,7 @@ var ( | |||
| reResourceGroupName = regexp.MustCompile(validResourceGroupNameRe) | |||
| reSnapshotName = regexp.MustCompile(`^[A-Za-z0-9_]{1,79}$`) | |||
| reSnapshotPrefix = regexp.MustCompile(`^[A-Za-z0-9_]{1,59}$`) | |||
| reResourceNamePrefix = regexp.MustCompile(validResourceNamePrefix) | |||
| reResourceNamePrefix = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9-]{1,9}$`) | |||
There was a problem hiding this comment.
Is there any reason why we reject names starting with -? From what I see in the docs it seems to be supported, unless I'm wrong, if so do correct me.
If that's so though, we can change the regex to [A-Za-z0-9-]{1,10}
There was a problem hiding this comment.
Come to think of it, if we want a one-character prefix, we can't accept it with your regex since it introduces a minimum of 2 characters, maybe the second part should be {0,9} if we maintain the logic behind validation
There was a problem hiding this comment.
Is there any reason why we reject names starting with
-? From what I see in the docs it seems to be supported, unless I'm wrong, if so do correct me. If that's so though, we can change the regex to[A-Za-z0-9-]{1,10}
I couldn't find anything in the documentation either, but when I try to create a VM starting with -, I get an error. So it's probably a parent rule that decides that:
Come to think of it, if we want a one-character prefix, we can't accept it with your regex since it introduces a minimum of 2 characters, maybe the second part should be {0,9} if we maintain the logic behind validation
Yeah, that's a oversight from my part. The intended regex would be ^[A-Za-z0-9][A-Za-z0-9-]{0,9}$. Is that okay?
There was a problem hiding this comment.
Hey @Swampen!
Sorry for letting that one slide, I completely miss your reroll, sorry for that.
The new additions look good to me, we'll merge that since tests are passing.
Thanks for your contribution, much appreciated.
Background
I discovered a few issues in the validation of the
custom_resource_build_prefixparameter.10_,.and)for Azure resourcespkr_123456is not a valid prefix name for a VM (because of the underscore) as stated by Microsoft's documentation:isValidAzureNamewould not allow the prefix to end with a-witch should be allowed according to the documentationcustom_resource_build_prefix+ resourcetype + 5 character random alphanumeric stringThis PR address the issues I described above as well as adding tests for
custom_resource_build_prefix