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.

Sign up for GitHub

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

Jump to bottom

Add --storage-id flag to Repo Create CLI #8557

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add --storage-id flag to Repo Create CLI #8557

wants to merge 14 commits into from

Conversation

itaigilo
Copy link
Contributor

Closes #8511.

Change Description

Support creating a repository with storageID in the CLI.

@itaigilo itaigilo added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached msb labels Jan 27, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 27, 2025 •
edited
Loading

🎊 PR Preview 2049918 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8557.surge.sh

🕐 Build time: 0.01s

🤖 By surge-preview

@github-actions GitHub Actions
Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

13 passed

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 27, 2025 •
edited
Loading

E2E Test Results - Quickstart

11 passed

N-o-Z
N-o-Z requested changes Jan 27, 2025
View reviewed changes
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.

Thanks!
Some comments regarding documentation +
Please add esti tests

I have some reservations on documenting this parameter ATM.
Also, lets have a discussion on this in a different forum

cmd/lakectl/cmd/repo_create.go Outdated
@@ -16,7 +16,7 @@ const (

// repoCreateCmd represents the create repo command
var repoCreateCmd = &cobra.Command{
Use: "create <repository URI> <storage namespace>",
Use: "create <repository URI> <storage namespace> [--storage-id <storage id>]",
Copy link
Member

Choose a reason for hiding this comment

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

Lets not document this flag ATM and open an issue to discuss that

cmd/lakectl/cmd/repo_create.go Outdated
@@ -48,6 +52,7 @@ var repoCreateCmd = &cobra.Command{
//nolint:gochecknoinits
func init() {
repoCreateCmd.Flags().StringP("default-branch", "d", DefaultBranch, "the default branch of this repository")
repoCreateCmd.Flags().String("storage-id", "", "the storage of this repository")
Copy link
Member

Choose a reason for hiding this comment

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

Suggest we make this a hidden flag for now

cmd/lakectl/cmd/repo_create_bare.go Outdated
@@ -10,7 +10,7 @@ import (

// repoCreateBareCmd represents the create repo command
var repoCreateBareCmd = &cobra.Command{
Use: "create-bare <repository URI> <storage namespace>",
Use: "create-bare <repository URI> <storage namespace> [--storage-id <storage id>]",
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as the "create" command

@itaigilo
Copy link
Contributor Author

@N-o-Z regarding esti -
repo create is already covered by esti.
I don't think there's any point of also verifying that--storage-id is passed with an empty value.

Any other esti tests you think should be added?

@itaigilo itaigilo requested a review from N-o-Z January 28, 2025 12:02
@N-o-Z
Copy link
Member

N-o-Z commented Feb 4, 2025

@N-o-Z regarding esti - repo create is already covered by esti. I don't think there's any point of also verifying that--storage-id is passed with an empty value.

Any other esti tests you think should be added?

We should test 2 things:

  1. Explicitly passing storage_id with "" value will create a valid repository
  2. Explicitly passing storage_id with any other value should fail

@itaigilo
Copy link
Contributor Author

itaigilo commented Feb 6, 2025

We should test 2 things:

  1. Explicitly passing storage_id with "" value will create a valid repository
  2. Explicitly passing storage_id with any other value should fail

Add the first,
And regarding the second - the CLI isn't failing it, but the Config validation.
So using RunCmdAndVerifyFailureWithFile doesn't work with a proper golden file, since it's not a CLI error.

@N-o-Z Can you think of a way to test this, or are you OK with having the Config tests covering this part?

N-o-Z
N-o-Z requested changes Feb 6, 2025
View reviewed changes
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.

Looks good.
Still missing a couple of things

cmd/lakectl/cmd/repo_create.go
@@ -52,7 +52,7 @@ var repoCreateCmd = &cobra.Command{
//nolint:gochecknoinits
func init() {
repoCreateCmd.Flags().StringP("default-branch", "d", DefaultBranch, "the default branch of this repository")
repoCreateCmd.Flags().String("storage-id", "", "the storage of this repository")
repoCreateCmd.Flags().String("storage-id", "", "")
Copy link
Member

Choose a reason for hiding this comment

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

Can you please define the storage-id flag globally (see withNoProgress as example) and set its property to hidden so it won't show up in the CLI docs (MarkHidden)

esti/lakectl_test.go Outdated
"BRANCH": mainBranch,
}
RunCmdAndVerifySuccessWithFile(t, Lakectl()+" repo create lakefs://"+repoNameSID+" "+storageSID+" --storage-id \"\"", false, "lakectl_repo_create", vars)

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to separate this into a different test.
Also, missing the failure scenario when running it with storage_id != ""

@itaigilo itaigilo requested a review from N-o-Z February 6, 2025 17:11
@itaigilo
Copy link
Contributor Author

itaigilo commented Feb 7, 2025

@N-o-Z fixed -
Can you PTAL again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nadavsteindler nadavsteindler Awaiting requested review from nadavsteindler

@guy-har guy-har Awaiting requested review from guy-har

@N-o-Z N-o-Z Awaiting requested review from N-o-Z

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached msb
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add --storage-id flag to repo create CLI
2 participants
@itaigilo @N-o-Z