-
Notifications
You must be signed in to change notification settings - Fork 362
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
Add StorageID to Repository entity #8502
base: master
Are you sure you want to change the base?
Conversation
😭 Deploy PR Preview 74e2202 failed. Build logs 🤖 By surge-preview |
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.
Solid work.
I have some comments that I think require our attention before approval.
As well as missing unit tests for graveler/catalog level
@@ -60,7 +60,8 @@ func GetBasicHandler(t *testing.T, authService *FakeAuthService, repoName string | |||
storageNamespace = "replay" | |||
} | |||
|
|||
_, err = c.CreateRepository(ctx, repoName, storageNamespace, "main", false) | |||
// TODO (gilo): test storageID here? |
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.
Can you explain? This is a test utility - what would you like to test here?
pkg/graveler/validate.go
Outdated
panic(ErrInvalidType) | ||
} | ||
|
||
// TODO (gilo): Any other validations? |
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 don't think this validation is required at all at this point. The storage ids already exist in the lakefs config so the parameter either matches one of them or it doesn't
We should however definitely define the requirements for a valid storage_id string and enforce during configuration load.
Lets just decide on min+max length + allowed chars and coordinate with @treeverse/product on this.
We should open an appropriate issue to enforce this validation.
pkg/catalog/catalog.go
Outdated
storageNS := graveler.StorageNamespace(storageNamespace) | ||
branchID := graveler.BranchID(branch) | ||
if err := validator.Validate([]validator.ValidateArg{ | ||
{Name: "name", Value: repositoryID, Fn: graveler.ValidateRepositoryID}, | ||
{Name: "storageID", Value: storageIdentifier, Fn: graveler.ValidateStorageID}, |
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.
No need for a validation on storage_id it's a given in the configuration at this point
@@ -2010,7 +2010,7 @@ func (c *Controller) CreateRepository(w http.ResponseWriter, r *http.Request, bo | |||
if swag.BoolValue(params.Bare) { | |||
// create a bare repository. This is useful in conjunction with refs-restore to create a copy | |||
// of another repository by e.g. copying the _lakefs/ directory and restoring its refs | |||
repo, err := c.Catalog.CreateBareRepository(ctx, body.Name, body.StorageNamespace, defaultBranch, swag.BoolValue(body.ReadOnly)) | |||
repo, err := c.Catalog.CreateBareRepository(ctx, body.Name, "", body.StorageNamespace, defaultBranch, swag.BoolValue(body.ReadOnly)) |
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.
Since only the blockstore is aware of the current configuration (using either blockstore
or blockstores
), we should have a validation at the adapter level which checks whether this value is "Valid". For OSS the validation should allow empty value (or enforce it - I don't really know if we should allow providing any storage_id in that case)
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.
Makes sense.
Removed this validation.
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.
@N-o-Z validation removed.
PTAL again.
Closes #8495.