Skip to content
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

Default storage namespace #2952

Merged
merged 5 commits into from
Feb 20, 2022
Merged

Conversation

johnnyaug
Copy link
Contributor

Added a new configuration blockstore.default_namespace_prefix.
When specified, the storage namespace field in the UI will be auto-filled.
The specified value will be the prefix, and the repository name will be added to it.

image

Extra

  • Added a (?) icon near the storage namespace field, linking to the docs.
  • Small docs improvements.

@johnnyaug johnnyaug added the include-changelog PR description should be included in next release changelog label Feb 17, 2022
Copy link
Contributor

@talSofer talSofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

The specified value will be the prefix, and the repository name will be added to it.

Why are we adding the repository name to the prefix? to create a separate storage space for each repository (if so, why)?

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat, thanks!

(Please also wait for OSS TF approval)

@@ -51,6 +51,9 @@ This reference uses `.` to denote the nesting of values.
* `auth.ldap.default_user_group` `(string : )` - Create all LDAP users in this group. Defaults to `Viewers`.
* `auth.ldap.user_filter` `(string : )` - Additional filter for users.
* `blockstore.type` `(one of ["local", "s3", "gs", "azure", "mem"] : required)`. Block adapter to use. This controls where the underlying data will be stored
* `blockstore.default_namespace_prefix` `(string : )` - Use this to help your users choose a storage namespace for their repositories.
When creating a repository from the UI, the storage namespace will be filled with this default value as a prefix.
The user may still change it to something else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If missing, ..."?

@@ -146,9 +146,13 @@ other user-defined merge strategies for handling conflicts are on the roadmap.
_Underlying storage_ is the area on some other object store that lakeFS uses to store object
contents and some of its metadata. We sometimes refer to underlying storage as _physical_.
The path used to store the contents of an object is then termed a _physical path_. The object
itself on underlying storage is never modified, except to remove it entirely during some
itself in the underlying storage is never modified, except to remove it entirely during some
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the original statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


const [storageNamespaceValid, setStorageNamespaceValid] = useState(true);
const [storageNamespaceValid, setStorageNamespaceValid] = useState(defaultNamespacePrefix ? true : null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the prefix itself is necessarily "valid"; don't we still need to check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! This is a leftover from a previous version

@@ -52,7 +60,7 @@ export const RepositoryCreateForm = ({ config, onSubmit, onCancel, error = null,
if (!formValid) {
return;
}
onSubmit({
onSubmit({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

<Form.Control type="text" autoFocus ref={repoNameField} onChange={checkRepoValidity}/>
{!repoValid &&
<Form.Control type="text" autoFocus ref={repoNameField} onChange={onRepoNameChange}/>
{repoValid === false &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{repoValid === false &&
{!repoValid &&

(think I prefer this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually fixing a bug where the "Create repository" button becomes enabled even if you don't fill the storage namespace.

@johnnyaug
Copy link
Contributor Author

johnnyaug commented Feb 17, 2022

Why are we adding the repository name to the prefix? to create a separate storage space for each repository (if so, why)?

@talSofer, yes.

Repositories should be in separate namespaces. We maybe not stress this enough in our docs, but some settings (like GC, branch protection) will override each other if repositories are in the same namespace.

@johnnyaug johnnyaug requested a review from talSofer February 17, 2022 15:11
@johnnyaug johnnyaug merged commit 0924792 into master Feb 20, 2022
@johnnyaug johnnyaug deleted the feature/default_namespace_prefix branch February 20, 2022 08:03
@talSofer
Copy link
Contributor

@talSofer, yes.
Repositories should be in separate namespaces. We maybe not stress this enough in our docs, but some settings (like GC, branch protection) will override each other if repositories are in the same namespace.

I see, can you open an issue to improve docs and maybe force this by code + migrate environments that are structured differently?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants