-
Notifications
You must be signed in to change notification settings - Fork 359
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
Fix create repo min character limit issue #3155
Fix create repo min character limit issue #3155
Conversation
@@ -15,7 +15,7 @@ const DEFAULT_BLOCKSTORE_VALIDITY_REGEX = new RegExp(`^s3://`); | |||
|
|||
export const RepositoryCreateForm = ({ config, onSubmit, onCancel, error = null, inProgress = false, sm = 6 }) => { | |||
const fieldNameOffset = 3; | |||
const repoValidityRegex = /^[a-z0-9][a-z0-9-]{2,62}$/; | |||
const repoValidityRegex = /^[a-z0-9][a-z0-9-]{1,62}$/; |
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 pattern was ok - based on "Bucket names must be between 3 (min) and 63 (max) characters long" like s3.
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.
Thanks @nopcoder I didn't realize original naming req's were based on s3 naming req's. Totally makes sense now. I'll update, rebase, and push corrections. Thank you!
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.
@nopcoder I was looking over s3 bucket naming rules and noticed s3 buckets must also adhere to these 2 specific requirements
Bucket names must not start with the prefix xn--.
Bucket names must not end with the suffix -s3alias
I noticed with the current regex iteration, users are able to make repo names that match these restrictions.
Should I improve the regex to avoid these 2 specific prefix and suffix restrictions? However, I am guessing I should not do that (and you all probably didn't do it originally) given lakeFS does not solely use S3, given you can also use Azure Storage which doesn't have these 2 specific restrictions. Is that correct?
If so, then I'm also curious what happens in the case I am using S3 as my underlying lakeFS storage and I make a bucket with that restricted prefix and/or restricted suffix?
Thanks in advance.
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.
FYI code has been updated so that wording now states that 3 characters is the minimum character limit for Repository ID.
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.
Thanks, @DataDavD, I would open a different issue for the extra validation as the enforcement should be first implemented on the backend.
Compatibility is part of S3 gateway, if all S3 clients or SDKs do not validate 'xn--' and '-s3alias' - lakefs can support it, as the limitation is from the underlying storage.
Currently, there exists a bug upon which the Create A New Repository form states that repo ID's must be at least 2 characters. However, it should state that the minimum is 3 characters given S3, Azure Storage, and Google Cloud Storage all have a 3 character minimum character limit for bucket naming. Closes treeverse#2967
77b25c5
to
9390b34
Compare
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!
thanks @nopcoder!!! Glad to get another lakeFS PR completed, regardless of how small it may be 😆 |
Currently, there exists a bug upon which the Create A New Repository
form states that repo ID's must be at least 2 characters, but in
actuality it does not allow less than 3. We update the regex string so
that the minimum is now 2 characters.
Closes #2967