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 storage helpers #2655

Merged
merged 39 commits into from
Mar 5, 2024
Merged

Conversation

plar
Copy link
Contributor

@plar plar commented Feb 6, 2024

Change Overview

This PR is the 3rd 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 core storage flag structures and storage helpers which will be used to build storage flags for specific providers(filesystem, s3, etc), along with extended error definitions.

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

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>
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>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>
Signed-off-by: pavel.larkin <pavel.larkin@veeam.com>

// HasSkipSSLVerify returns true if the location has skip SSL verification.
func (l Location) HasSkipSSLVerify() bool {
v, _ := strconv.ParseBool(string(l[rs.SkipSSLVerifyKey]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we handle error?

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've implemented the same logic as it is in the current implementation:

// pkg/kopia/command/storage/secret_utils.go
func checkSkipSSLVerifyFromMap(m map[string][]byte) bool {
	v := string(m[repositoryserver.SkipSSLVerifyKey])
	return v == "true"
}

strconv.ParseBool returns false in the case of error.

"github.com/kanisterio/kanister/pkg/log"
)

// NopLogger is a logger that does nothing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

@plar plar Feb 29, 2024

Choose a reason for hiding this comment

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

This Nop logger is used inside s3 storage as a default logger if no logger is provided.
It helps to avoid code cluttering and remove some code bumps.

You don't need to write something like below:

    if logger != nil {
        logger.Print("Removing trailing slashes from the endpoint")
    }

and can simply use:

    logger.Print("Removing trailing slashes from the endpoint")

// If the location-specific prefix is empty, the repository-specific prefix is returned.
func GenerateFullRepoPath(locPrefix, repoPathPrefix string) string {
if locPrefix != "" {
return path.Join(locPrefix, repoPathPrefix) + "/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is trailing / compulsory? What's the problem if we don't add?

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 code is copied from pkg/kopia/command/storage/utils.go. @ankitjain235, can you help us understand why a trailing slash is needed here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's needed, but will let @pavannd1 confirm. Anyways it's existing behaviour so should be good to merge, we can remove it if not required later?

pkg/kopia/cli/internal/location_test.go Outdated Show resolved Hide resolved
pkg/kopia/cli/internal/path_test.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>
@plar plar requested a review from PrasadG193 March 1, 2024 22:57
Base automatically changed from kopia-cli-common-flags to master March 5, 2024 18:48
@plar plar added the kueue label Mar 5, 2024
@mergify mergify bot merged commit 30e4f5c 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-storage-core-flags branch March 5, 2024 20:00
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

5 participants