-
Notifications
You must be signed in to change notification settings - Fork 158
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
Conversation
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
There was a problem hiding this 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.
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
There was a problem hiding this 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.
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>
3567381
to
dcd6425
Compare
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
There was a problem hiding this 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
Argument: args.ID("id12345"), | ||
ExpectedCLI: []string{"cmd", "id12345"}, | ||
}, | ||
}}) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
ContentCacheSizeMB int // the size of the content cache in MB. | ||
MetadataCacheSizeMB int // the size of the metadata cache in MB. |
There was a problem hiding this comment.
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?
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:
This PR introduces common argument structures and flags for Kopia CLI, along with extended error definitions:
pkg/kopia/cli/args/Common
andpkg/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:
Test Plan