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

Implement Kopia common args and opts #2654

Merged
merged 29 commits into from
Mar 5, 2024
Merged

Implement Kopia common args and opts #2654

merged 29 commits into from
Mar 5, 2024

Conversation

plar
Copy link
Contributor

@plar plar commented Feb 6, 2024

Change Overview

This PR is the 2nd in the series of PRs that will add a new way to build Kopia CLI commands.

PR Train:

  1. PR-2653 Introducing a new approach to building Kopia CLI commands using safecli v0.0.6
  2. PR-2654 Implement Kopia common args and opts
  3. PR-2655 Implement Kopia storage helpers
  4. PR-2656 Add Kopia Filesystem storage opts
  5. PR-2657 Add Kopia GCS storage opts
  6. PR-2658 Add Kopia Azure storage opts
  7. PR-2659 Add Kopia S3 and S3 compliant storage opts
  8. PR-2661 Add Kopia repository create command
  9. PR-2662 Add Kopia repository connect command
  10. PR-2663 Add Kopia repository connect server command
  11. PR-2664 Add Kopia repository set-parameters command
  12. PR-2665 Add Kopia repository status command
    This PR introduces common argument structures and flags for Kopia CLI, along with extended error definitions:
  • pkg/kopia/cli/args/Common and pkg/kopia/cli/args/CacheA structs encapsulate common command-line arguments and cache-related arguments for the Kopia CLI.
  • pkg/kopia/cli/internal/opts package presents common and cache-related options for the Kopia CLI.
  • pkg/kopia/cli/internal/args package presents common arguments for the Kopia CLI.

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Copy link
Contributor

@hairyhum hairyhum left a comment

Choose a reason for hiding this comment

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

Just some comments about code size. It feels like we can shed some code in some places here.

pkg/kopia/cli/internal/flag/common/common_flags.go Outdated Show resolved Hide resolved
pkg/kopia/cli/internal/flag/common/common_flags.go Outdated Show resolved Hide resolved
pkg/kopia/cli/internal/flag/common/common_flags.go Outdated Show resolved Hide resolved
pkg/kopia/cli/internal/flag/common/common_flags.go Outdated Show resolved Hide resolved
plar and others added 3 commits February 12, 2024 12:11
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Copy link
Contributor

@ankitjain235 ankitjain235 left a comment

Choose a reason for hiding this comment

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

Added one suggestion, looks good otherwise.

pkg/kopia/cli/internal/flag/common/common_flags.go Outdated Show resolved Hide resolved
pkg/kopia/cli/internal/flag/common/common_flags.go Outdated Show resolved Hide resolved
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Kanister automation moved this from In Progress to Reviewer approved Feb 28, 2024
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Copy link
Contributor

@PrasadG193 PrasadG193 left a comment

Choose a reason for hiding this comment

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

Needs minor formating fixes, rest looks good to me

pkg/kopia/cli/internal/args/args_test.go Outdated Show resolved Hide resolved
Argument: args.ID("id12345"),
ExpectedCLI: []string{"cmd", "id12345"},
},
}})
Copy link
Contributor

Choose a reason for hiding this comment

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

Not test to run defined? Am I missing 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.

What do you mean? We have 2 tests here:

func TestArgs(t *testing.T) { check.TestingT(t) }
// Go test/bridge func for `go test` and delegate the actual testing work to the `check` framework.

// register `test.ArgumentSuite` with the `check` framework.
var _ = check.Suite(&test.ArgumentSuite{Cmd: "cmd", Arguments: []test.ArgumentTest{
	{
		Name:        "Invalid ID",
		Argument:    args.ID(""),
		ExpectedErr: cli.ErrInvalidID,
	},
	{
		Name:        "ID",
		Argument:    args.ID("id12345"),
		ExpectedCLI: []string{"cmd", "id12345"},
	},
}})

We're using github.com/kanisterio/safecli/test.ArgumentSuite to run table tests.

pkg/kopia/cli/internal/opts/cache_opts.go Show resolved Hide resolved
pkg/kopia/cli/internal/opts/common_opts.go Show resolved Hide resolved
pkg/kopia/cli/internal/opts/opts_test.go Show resolved Hide resolved
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
@plar plar requested a review from PrasadG193 March 1, 2024 22:55
Base automatically changed from kopia-cli-core-infra to master March 5, 2024 17:33
@plar plar added the kueue label Mar 5, 2024
@mergify mergify bot merged commit 15dff38 into master Mar 5, 2024
14 checks passed
Kanister automation moved this from Reviewer approved to Done Mar 5, 2024
@mergify mergify bot deleted the kopia-cli-common-flags branch March 5, 2024 18:48
Comment on lines +24 to +25
ContentCacheSizeMB int // the size of the content cache in MB.
MetadataCacheSizeMB int // the size of the metadata cache in MB.
Copy link
Contributor

Choose a reason for hiding this comment

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

@plar If these are not referenced anywhere, can you remove them please?

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

Successfully merging this pull request may close these issues.

None yet

7 participants