support setting ImageFamily and BootMode when building image #140
Conversation
|
|
There was a problem hiding this comment.
Hi @hwhuangzsly,
Thanks for the PR, I left a suggestion regarding validating the image family's contents before doing the build, may I also suggest adding unit tests for the config? It would be useful to make sure the invalid cases are caught during Prepare.
I'll let you address those, feel free to ping me when you're done (or if you have questions), and I'll do another pass then.
Thanks again!
hwhuangzsly
force-pushed
the
support_set_image_family
branch
2 times, most recently
from
d7929aa to
4ee3aa7
Compare
|
@lbajolet-hashicorp Thanks for your suggestions. I have added tests to the "target_image_family" and "boot_mode", and optimize the verification method and description of the "target_image_family". Please take a review again~ |
There was a problem hiding this comment.
Hey @hwhuangzsly,
Thanks for the reroll! I've left only a couple of comments left on the regex and the tests, but overall this looks good to me, I'll let you address those and we can come back for another round of reviews then.
Pre-approving so it's not a problem later, the code is near-ready I think.
builder/ecs/image_config_test.go
Outdated
| } | ||
|
|
||
| // success | ||
| c.AlicloudTargetImageFamily = "success" |
There was a problem hiding this comment.
Just to be sure the \p{L} expression captures that, mind adding a test with chinese characters since they're explicitly documented as valid for the URIs?
There was a problem hiding this comment.
I add new tests to ensure the regex can match Chinese characters, -, _, : and .
builder/ecs/image_config.go
Outdated
| strings.HasPrefix(c.AlicloudTargetImageFamily, "aliyun") { | ||
| errs = append(errs, fmt.Errorf("target_image_family can't start with 'aliyun', 'acs:', 'http://' or 'https://'")) | ||
| } else { | ||
| imageFamilyReg := regexp.MustCompile(`^[a-zA-Z\p{L}][a-zA-Z\p{L}_0-9\-\.\:]{1,127}$`) |
There was a problem hiding this comment.
I don't know all my unicode character classes by heart, but doesn't L also match the a-zA-Z ranges? We can probably simplify that expression if that's the case
| imageFamilyReg := regexp.MustCompile(`^[a-zA-Z\p{L}][a-zA-Z\p{L}_0-9\-\.\:]{1,127}$`) | |
| imageFamilyReg := regexp.MustCompile(`^\p{L}[\p{L}_0-9\-\.\:]{1,127}$`) |
I believe also that both . and : don't need escaping in the context of a []? Not sure though, that probably depends on the regex engine.
There was a problem hiding this comment.
After testing, I find that p{L} can match a-zA-Z but can't match . or digit, so I simplify it as your suggestion. It's pleasure for me to learn more about the regex rule.
hwhuangzsly
force-pushed
the
support_set_image_family
branch
2 times, most recently
from
5332248 to
3c89f8c
Compare
hwhuangzsly
force-pushed
the
support_set_image_family
branch
from
3c89f8c to
6a8427f
Compare
|
The last reroll looks good to me @hwhuangzsly, thanks for pushing this one forward! I'll wait for tests to go green and merge this. |
|
@lbajolet-hashicorp Excuse me, do you know when the next release version will be made? |
|
Hi @hwhuangzsly, This can happen reasonably soon I would think, maybe today or tomorrow. I'll let you know when this happens |
|
@lbajolet-hashicorp Excuse me, as we previously mentioned, the next release version will be made soon. I think maybe you forget it, just a gentle reminder. |
|
Hi @hwhuangzsly, Thanks for the reminder, I did forget indeed. |
support setting ImageFamily and BootMode when building image