-
Notifications
You must be signed in to change notification settings - Fork 93
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
Adding name convention validator for QHub project name #761
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.
Edited text for grammar/tone.
Using ALL CAPS FEELS LIKE YELLING :)
Also, the phrase "for starters" has connotations that there's a big list of things incoming, and sends the wrong message when indicating the user did something wrong (this could be a person preference thing)
Also suggested edits to allow -
in the names
Co-authored-by: Tyler <49161327+tylerpotts@users.noreply.github.com>
Co-authored-by: Tyler <49161327+tylerpotts@users.noreply.github.com>
Co-authored-by: Tyler <49161327+tylerpotts@users.noreply.github.com>
Co-authored-by: Tyler <49161327+tylerpotts@users.noreply.github.com>
Co-authored-by: Tyler <49161327+tylerpotts@users.noreply.github.com>
- letters from A to Z (upper and lower case) and numbers; | ||
- Special characters are not allowed, such as "!@#$%^&*()"; | ||
- Maximum accepted length of the name string is 16 characters. | ||
- If using AWS: names should not start with the string "aws"; |
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.
Where does this come from? It looks like the current aws test has a project name aws-test
This is causing the AWS tests to fail
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.
It is the description... but I don't remember why we need to avoid that. I remember a discussion about that a really long time ago... Anyways I will need to add another step in that validation because the Azure storage name must not contain any dashes
qhub/schema.py
Outdated
raise ValueError( | ||
"Maximum accepted length of the project name string is 16 characters." | ||
) | ||
elif re.findall(r"^(?!aws)[A-Za-z0-9][^/|.~!?@#$%^=&*\\()_]*[A-Za-z0-9]$", value): |
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.
@tarundmsharma so right now this regex is not considering all dashes in the project name, which is not something we would like to have for all providers... only for Azure. So we need to add a conditional that checks the target provider and use the correct regex based on it.
@tarundmsharma this PR is failing due to the tests having |
sure @costrouc, thanks for the direction. ill check. but logically thinking, i had tried PR for only the code which had validator for azure, still the tests failed. can it be because of "-". if yes then do i have to do any further changes to eliminate the failure because of "-". |
qhub/schema.py
Outdated
"Maximum accepted length of the project name string is 16 characters." | ||
) | ||
elif ProviderEnum == AzureProvider and re.findall( | ||
r"^[A-Za-z0-9][^/|.~!?@#$%^=&*\\()_]*[A-Za-z0-9]$", value |
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.
@viniciusdc and @tarundmsharma I'm not sure what this regular expression is meant to do. Could you explain?
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 part of code is checking the project name can have A-Z, a-z, 0-9 and special character, if the project name contains characters beyond the list. error will be given during the deployments.
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.
Hi @costrouc originally, this was the complete regex #761 (comment), it was supposed to return only strings that do NOT contain aws
in the beginning of the project name as they are alphanumeric (not including extra characters). If one of those conditions do not hold, then the find all will return an empty list and an error will be raised. https://github.com/Quansight/qhub/blob/af8323ce27196254b88de065023fcbb60953d292/qhub/schema.py#L388
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'd rather have a simple regex and rules for users on what can be in a project name so that we don't have to have so many exceptions. For example:
- must begin with a letter
[a-z][A-Z]
- cannot have name begin with
aws
- name can only contain
[a-zA-Z0-9]
Can you make this the rule for checking? Is this sane?
@viniciusdc once #927 is approved and merged, I can go back and resolve any merge conflicts here 👍 |
@viniciusdc this is ready for another review when you get the chance, thanks :) |
This is now up-to-date with main, though this now requires a review of our naming conventions to continue. I will open a tracking issue to research this and update here once finished. |
@viniciusdc could you update the tests? |
In short, as QHub utilizes different cloud providers each one has a number of constraints that must be considered while naming resources. Because of that, QHub currently uses some naming conventions to better address these outstanding restrictions. Using this validator we will avoid issues with QHub deployment as well give more feedback to the end user.
QHub naming convetion used as base.
close #626