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

itaigilo · 2025-01-27T13:57:55Z

Closes #8511.

Change Description

Support creating a repository with storageID in the CLI.

github-actions · 2025-01-27T13:59:20Z

🎊 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 · 2025-01-27T14:04:20Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-01-27T14:04:56Z

E2E Test Results - Quickstart

N-o-Z

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

N-o-Z · 2025-01-27T17:18:20Z

cmd/lakectl/cmd/repo_create.go

@@ -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>]",


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

N-o-Z · 2025-01-27T17:19:06Z

cmd/lakectl/cmd/repo_create.go

@@ -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")


Suggest we make this a hidden flag for now

N-o-Z · 2025-01-27T17:19:43Z

cmd/lakectl/cmd/repo_create_bare.go

@@ -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>]",


Same comments as the "create" command

itaigilo · 2025-01-28T12:02:00Z

@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?

N-o-Z · 2025-02-04T19:05:09Z

@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:

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

itaigilo · 2025-02-06T06:32:28Z

We should test 2 things:

Explicitly passing storage_id with "" value will create a valid repository

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

Looks good.
Still missing a couple of things

N-o-Z · 2025-02-06T16:45:04Z

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", "", "")


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)

N-o-Z · 2025-02-06T16:47:31Z

esti/lakectl_test.go

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


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 · 2025-02-07T06:58:47Z

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

# Conflicts: # cmd/lakectl/cmd/repo_create.go # docs/reference/cli.md

nadavsteindler

Looks good- added a few comments

nadavsteindler · 2025-02-10T08:54:03Z

cmd/lakectl/cmd/repo_create.go

+
+		defaultBranch, err := cmd.Flags().GetString(defaultBranchFlagName)
+		if err != nil {
+			DieErr(err)


What is the difference between the Must and this error handling? What is the reason for this change?

I think it's some merges mixed up.
Updating.

Following a talk with @N-o-Z -
Changed it to Must. Same effect, also for optional params.

nadavsteindler · 2025-02-10T08:58:41Z

cmd/lakectl/cmd/repo_create.go

+			DieErr(err)
+		}
+		sampleData := Must(cmd.Flags().GetBool(sampleDataFlagName))
+		storageID, _ := cmd.Flags().GetString(storageIDFlagName)


Why ignore the error?

Since it's an optional param.

There are other possible errors than "flag accessed but not defined"

Thanks.
Added error handling for this (obviously, copied from other commands).

nadavsteindler · 2025-02-10T08:59:47Z

cmd/lakectl/cmd/root.go

@@ -187,6 +188,13 @@ func withRecursiveFlag(cmd *cobra.Command, usage string) {
 	cmd.Flags().BoolP(recursiveFlagName, recursiveFlagShort, false, usage)
 }

+func withStorageID(cmd *cobra.Command) {


Clean code- add a one line comment explaining what this does

Isn't clean code about self-explained code?

And again, StorageID is not very documented in the OSS at the moment,
And this a part of it.

OK, hmm I find it takes me a while to read this function and understand what I'm looking at. Maybe a better function name rather than a comment? Maybe it should be addStorageID? withStorageID sounds like it's more of a query checking if the command has this flag than a command that does something

This is the convention for global flags in lakectl...
(See @N-o-Z 's comment on this PR.)

itaigilo

@nadavsteindler thanks.
Addressed - PTAL again.

itaigilo · 2025-02-10T09:34:22Z

cmd/lakectl/cmd/repo_create.go

+			DieErr(err)
+		}
+		sampleData := Must(cmd.Flags().GetBool(sampleDataFlagName))
+		storageID, _ := cmd.Flags().GetString(storageIDFlagName)


Since it's an optional param.

itaigilo · 2025-02-10T09:42:57Z

cmd/lakectl/cmd/root.go

@@ -187,6 +188,13 @@ func withRecursiveFlag(cmd *cobra.Command, usage string) {
 	cmd.Flags().BoolP(recursiveFlagName, recursiveFlagShort, false, usage)
 }

+func withStorageID(cmd *cobra.Command) {


Isn't clean code about self-explained code?

And again, StorageID is not very documented in the OSS at the moment,
And this a part of it.

itaigilo · 2025-02-10T09:43:38Z

cmd/lakectl/cmd/repo_create.go

+
+		defaultBranch, err := cmd.Flags().GetString(defaultBranchFlagName)
+		if err != nil {
+			DieErr(err)


I think it's some merges mixed up.
Updating.

N-o-Z

Thanks!
Approved with 2 comments that need to be addressed

N-o-Z · 2025-02-10T15:05:53Z

cmd/lakectl/cmd/repo_create.go

+
+		defaultBranch := Must(cmd.Flags().GetString(defaultBranchFlagName))
+		sampleData := Must(cmd.Flags().GetBool(sampleDataFlagName))
+		storageID, err := cmd.Flags().GetString(storageIDFlagName)


Should also be under a Must

Isn't the --storage flag optional for the OSS?

N-o-Z · 2025-02-10T15:06:29Z

cmd/lakectl/cmd/repo_create_bare.go

-		}
+
+		defaultBranch := Must(cmd.Flags().GetString(defaultBranchFlagName))
+		storageID, _ := cmd.Flags().GetString(storageIDFlagName)


Add --storage-id flag to Repo Create CLI

Loading
Loading status checks…

ad9e32c

itaigilo added exclude-changelog minor-change msb labels Jan 27, 2025

itaigilo requested review from N-o-Z, nadavsteindler and guy-har January 27, 2025 13:57

N-o-Z requested changes Jan 27, 2025

View reviewed changes

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

itaigilo added 7 commits February 3, 2025 17:05

Remove flag documentation

94a246c

Add esti tests

Loading
Loading status checks…

5411788

Fix tests

Loading
Loading status checks…

9a76eac

Fix esti tests

Loading
Loading status checks…

c7ac2e2

Fix esti test

Loading
Loading status checks…

efe5832

Fix tests

Loading
Loading status checks…

44a2706

Fix tests

Loading
Loading status checks…

c5bdb3c

itaigilo added 3 commits February 5, 2025 17:25

Update the tests

Loading
Loading status checks…

06aac0f

Merge branch 'master' into feature/add-storage-id-to-cli

Loading
Loading status checks…

71e445a

Fix tests

Loading
Loading status checks…

3700ede

N-o-Z requested changes Feb 6, 2025

View reviewed changes

itaigilo requested a review from N-o-Z February 6, 2025 17:11

itaigilo added 3 commits February 6, 2025 21:59

Update tests

Loading
Loading status checks…

c6b4cbb

Merge branch 'master' into feature/add-storage-id-to-cli

Loading
Loading status checks…

8b7c88d

Fix test file

Loading
Loading status checks…

2049918

itaigilo added 2 commits February 8, 2025 21:12

Make flag hidden

Loading
Loading status checks…

4159a45

Merge branch 'master' into feature/add-storage-id-to-cli

Loading
Loading status checks…

0e64d9e

# Conflicts: # cmd/lakectl/cmd/repo_create.go # docs/reference/cli.md

nadavsteindler requested changes Feb 10, 2025

View reviewed changes

Fix PR comments

Loading
Loading status checks…

9242822

itaigilo commented Feb 10, 2025

View reviewed changes

Handle param error

Loading
Loading status checks…

4b19558

itaigilo requested a review from nadavsteindler February 10, 2025 15:00

nadavsteindler approved these changes Feb 10, 2025

View reviewed changes

N-o-Z approved these changes Feb 10, 2025

View reviewed changes

Use Must for flag

Loading
Loading status checks…

382207a

itaigilo enabled auto-merge (squash) February 10, 2025 17:46

itaigilo merged commit 502432d into master Feb 10, 2025
38 checks passed

itaigilo deleted the feature/add-storage-id-to-cli branch February 10, 2025 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

itaigilo commented Jan 27, 2025

github-actions bot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025

github-actions bot commented Jan 27, 2025 •

edited

Loading

N-o-Z left a comment

N-o-Z Jan 27, 2025

N-o-Z Jan 27, 2025

N-o-Z Jan 27, 2025

itaigilo commented Jan 28, 2025

N-o-Z commented Feb 4, 2025

itaigilo commented Feb 6, 2025

N-o-Z left a comment

N-o-Z Feb 6, 2025

N-o-Z Feb 6, 2025

itaigilo commented Feb 7, 2025

nadavsteindler left a comment

nadavsteindler Feb 10, 2025

itaigilo Feb 10, 2025

itaigilo Feb 10, 2025

nadavsteindler Feb 10, 2025

itaigilo Feb 10, 2025

nadavsteindler Feb 10, 2025

itaigilo Feb 10, 2025

nadavsteindler Feb 10, 2025

itaigilo Feb 10, 2025

nadavsteindler Feb 10, 2025

itaigilo Feb 10, 2025

itaigilo left a comment

itaigilo Feb 10, 2025

itaigilo Feb 10, 2025

itaigilo Feb 10, 2025

N-o-Z left a comment

N-o-Z Feb 10, 2025

itaigilo Feb 10, 2025

N-o-Z Feb 10, 2025

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

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

Conversation

itaigilo commented Jan 27, 2025

Change Description

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

github-actions bot commented Jan 27, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

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

E2E Test Results - Quickstart

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Jan 28, 2025

N-o-Z commented Feb 4, 2025

itaigilo commented Feb 6, 2025

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Feb 7, 2025

nadavsteindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2025 •

edited

Loading

github-actions bot commented Jan 27, 2025 •

edited

Loading