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

added storage namespace validation on non-readonly bare repo creation #8364

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tkalir
Copy link

@tkalir tkalir commented Nov 13, 2024

Closes #3165

Background

Currently bare repo creation doesn't check if namesapce storage is writable \ if a lakefs repo already exists there

Bug Fix

In Controller.go\CreateRepository, previously the code was -

  1. if bare repo, create and return
  2. if non-readonly repo, do namespace validation
    I only switched them places, so namespace validation happens also for non-readonly bare repos. nothing else is supposed to change. (but that does mean that bare repos create a dummy object at namespace root now, like other repos - see Controller.go\ensureStorageNamespace)

Testing Details

I added a system test to check that creating a bare repo in same namespace as existing repo returns an error. we can remove the test if it's unnecessary, but it will help to check the code.

Contact Details

[email protected] \ Tamar Kalir on slack

@tkalir tkalir marked this pull request as ready for review November 16, 2024 14:57
@N-o-Z N-o-Z self-requested a review November 21, 2024 18:01
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

Hi @tkalir great job!

Though this is a small PR I'm requesting changes for the test as this is your first PR and I think this can be a good learning experience going into the lakeFS code.

@@ -215,6 +215,24 @@ func TestController_GetRepoHandler(t *testing.T) {
})
}

t.Run("use same storage namespace twice with bare repo", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

That's a great test - but note you could have reused the code of "use same storage namespace twice" and parameterize the test.
Look at the example of catalog_test.go:564 as reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating bare repository skips storage namespace validation
2 participants