-
Notifications
You must be signed in to change notification settings - Fork 69
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
appservice: Fix containerized function app name validation to exclude uppercase letters #1754
Conversation
@@ -113,6 +113,8 @@ export class SiteNameStep extends AzureNameStep<SiteNameStepWizardContext> { | |||
|
|||
if (name.length < siteNamingRules.minLength || name.length > siteNamingRules.maxLength) { | |||
return vscode.l10n.t('The name must be between {0} and {1} characters.', siteNamingRules.minLength, siteNamingRules.maxLength); | |||
} else if ((!/^[a-z]([-a-z0-9]*[a-z0-9])?$/.test(name)) && this._siteFor === "containerizedFunctionApp") { |
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.
nit: You should check the siteFor property first so the conditional can just abort if it's not a containerized app. It's probably small, but still a wasted check.
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 go ahead and bump so we can release and add to Functions asap.
@@ -113,6 +113,8 @@ export class SiteNameStep extends AzureNameStep<SiteNameStepWizardContext> { | |||
|
|||
if (name.length < siteNamingRules.minLength || name.length > siteNamingRules.maxLength) { | |||
return vscode.l10n.t('The name must be between {0} and {1} characters.', siteNamingRules.minLength, siteNamingRules.maxLength); | |||
} else if (this._siteFor === "containerizedFunctionApp" && (!/^[a-z]([-a-z0-9]*[a-z0-9])?$/.test(name))) { |
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.
I think the *
here might be in the wrong spot, otherwise you can still have multiple -
's together. Here is what I think you had for ContainerAppNameStep
!
!/^[a-z][a-z0-9]*(-[a-z0-9]+)*$/.test(name)
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.
Oh lol yes I copied this from ManagedEnvironmentNameStep
since I figured it was the same since the error message was the same but I can change it.
On another note shouldn't the regex be the same between ContainerAppNameStep
and ManagedEnvironmentNameStep
since the error message is the same between the two?
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.
Yeah good catch, if the requirements are the same I think we should update that check logic
This is what the name validation on the portal now says:

It matches the validation from container apps so using the regular expression from there.