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

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

Merged
merged 19 commits into from
Feb 10, 2025
+43 −12
Merged

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
ad9e32c
Add --storage-id flag to Repo Create CLI
itaigilo Jan 27, 2025
94a246c
Remove flag documentation
itaigilo Feb 3, 2025
5411788
Add esti tests
itaigilo Feb 3, 2025
9a76eac
Fix tests
itaigilo Feb 4, 2025
c7ac2e2
Fix esti tests
itaigilo Feb 4, 2025
efe5832
Fix esti test
itaigilo Feb 4, 2025
44a2706
Fix tests
itaigilo Feb 4, 2025
c5bdb3c
Fix tests
itaigilo Feb 4, 2025
06aac0f
Update the tests
itaigilo Feb 5, 2025
71e445a
Merge branch 'master' into feature/add-storage-id-to-cli
itaigilo Feb 6, 2025
3700ede
Fix tests
itaigilo Feb 6, 2025
c6b4cbb
Update tests
itaigilo Feb 6, 2025
8b7c88d
Merge branch 'master' into feature/add-storage-id-to-cli
itaigilo Feb 6, 2025
2049918
Fix test file
itaigilo Feb 6, 2025
4159a45
Make flag hidden
itaigilo Feb 8, 2025
0e64d9e
Merge branch 'master' into feature/add-storage-id-to-cli
itaigilo Feb 8, 2025
9242822
Fix PR comments
itaigilo Feb 10, 2025
4b19558
Handle param error
itaigilo Feb 10, 2025
382207a
Use Must for flag
itaigilo Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 14 additions & 7 deletions cmd/lakectl/cmd/repo_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import (
)

const (
DefaultBranch = "main"

SampleDataFlag = "sample-data"
defaultBranchFlagName = "default-branch"
defaultBranchFlagValue = "main"
sampleDataFlagName = "sample-data"

repoCreateCmdArgs = 2
)
Expand All @@ -27,13 +27,19 @@ var repoCreateCmd = &cobra.Command{
clt := getClient()
u := MustParseRepoURI("repository URI", args[0])
fmt.Println("Repository:", u)
defaultBranch := Must(cmd.Flags().GetString("default-branch"))
sampleData := Must(cmd.Flags().GetBool(SampleDataFlag))

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

Choose a reason for hiding this comment

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

Should also be under a Must

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if err != nil {
DieErr(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

resp, err := clt.CreateRepositoryWithResponse(cmd.Context(),
&apigen.CreateRepositoryParams{},
apigen.CreateRepositoryJSONRequestBody{
Name: u.Repository,
StorageId: &storageID,
StorageNamespace: args[1],
DefaultBranch: &defaultBranch,
SampleData: &sampleData,
Expand All @@ -53,8 +59,9 @@ var repoCreateCmd = &cobra.Command{

//nolint:gochecknoinits
func init() {
repoCreateCmd.Flags().StringP("default-branch", "d", DefaultBranch, "the default branch of this repository")
repoCreateCmd.Flags().Bool(SampleDataFlag, false, "create sample data in the repository")
repoCreateCmd.Flags().StringP(defaultBranchFlagName, "d", defaultBranchFlagValue, "the default branch of this repository")
repoCreateCmd.Flags().Bool(sampleDataFlagName, false, "create sample data in the repository")
withStorageID(repoCreateCmd)

repoCmd.AddCommand(repoCreateCmd)
}
13 changes: 8 additions & 5 deletions cmd/lakectl/cmd/repo_create_bare.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@ var repoCreateBareCmd = &cobra.Command{
clt := getClient()
u := MustParseRepoURI("repository URI", args[0])
fmt.Println("Repository:", u)
defaultBranch, err := cmd.Flags().GetString("default-branch")
if err != nil {
DieErr(err)
}

defaultBranch := Must(cmd.Flags().GetString(defaultBranchFlagName))
storageID, _ := cmd.Flags().GetString(storageIDFlagName)
Copy link
Member

Choose a reason for hiding this comment

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

Must


bareRepo := true

resp, err := clt.CreateRepositoryWithResponse(cmd.Context(), &apigen.CreateRepositoryParams{
Bare: &bareRepo,
}, apigen.CreateRepositoryJSONRequestBody{
DefaultBranch: &defaultBranch,
Name: u.Repository,
StorageId: &storageID,
StorageNamespace: args[1],
})
DieOnErrorOrUnexpectedStatusCode(resp, err, http.StatusCreated)
Expand All @@ -43,7 +45,8 @@ var repoCreateBareCmd = &cobra.Command{

//nolint:gochecknoinits
func init() {
repoCreateBareCmd.Flags().StringP("default-branch", "d", DefaultBranch, "the default branch name of this repository (will not be created)")
repoCreateBareCmd.Flags().StringP(defaultBranchFlagName, "d", defaultBranchFlagValue, "the default branch name of this repository (will not be created)")
withStorageID(repoCreateBareCmd)

repoCmd.AddCommand(repoCreateBareCmd)
}
8 changes: 8 additions & 0 deletions cmd/lakectl/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ var (
const (
recursiveFlagName = "recursive"
recursiveFlagShort = "r"
storageIDFlagName = "storage-id"
presignFlagName = "pre-sign"
parallelismFlagName = "parallelism"
noProgressBarFlagName = "no-progress"
Expand Down Expand Up @@ -187,6 +188,13 @@ func withRecursiveFlag(cmd *cobra.Command, usage string) {
cmd.Flags().BoolP(recursiveFlagName, recursiveFlagShort, false, usage)
}

func withStorageID(cmd *cobra.Command) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 the convention for global flags in lakectl...
(See @N-o-Z 's comment on this PR.)

cmd.Flags().String(storageIDFlagName, "", "")
if err := cmd.Flags().MarkHidden(storageIDFlagName); err != nil {
DieErr(err)
}
}

func withParallelismFlag(cmd *cobra.Command) {
cmd.Flags().IntP(parallelismFlagName, "p", defaultParallelism,
"Max concurrent operations to perform")
Expand Down
3 changes: 3 additions & 0 deletions esti/golden/lakectl_repo_create_with_storage_id.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Repository: lakefs://${REPO}
storage id: invalid value: validation error
400 Bad Request
13 changes: 13 additions & 0 deletions esti/lakectl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,19 @@ func TestLakectlBasicRepoActions(t *testing.T) {
RunCmdAndVerifySuccessWithFile(t, Lakectl()+" repo create lakefs://"+repoName3+" "+storage3+" --sample-data", false, "lakectl_repo_create_sample", vars)
}

func TestLakectlRepoCreateWithStorageID(t *testing.T) {
// Validate the --storage-id flag (currently only allowed to be empty)
repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)
vars := map[string]string{
"REPO": repoName,
"STORAGE": storage,
"BRANCH": mainBranch,
}
RunCmdAndVerifyFailureWithFile(t, Lakectl()+" repo create lakefs://"+repoName+" "+storage+" --storage-id storage1", false, "lakectl_repo_create_with_storage_id", vars)
RunCmdAndVerifySuccessWithFile(t, Lakectl()+" repo create lakefs://"+repoName+" "+storage+" --storage-id \"\"", false, "lakectl_repo_create", vars)
}

func TestLakectlPreSignUpload(t *testing.T) {
repoName := generateUniqueRepositoryName()
storage := generateUniqueStorageNamespace(repoName)
Expand Down
Loading