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

Kanister efs backups remove reference to k10 #1328

Merged
merged 11 commits into from
Mar 29, 2022

Conversation

leuyentran
Copy link
Contributor

@leuyentran leuyentran commented Mar 24, 2022

Change Overview

  1. Remove k10 references in aws efs and expose CreateBackupVaultWrapper for derivative work.
  2. Added SnapshotsListWLimit function

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

@shuguet shuguet added this to To do in Kanister via automation Mar 24, 2022
@@ -92,7 +90,7 @@ func NewEFSProvider(ctx context.Context, config map[string]string) (blockstorage

efsVault, ok := config[awsconfig.ConfigEFSVaultName]
if !ok || efsVault == "" {
efsVault = defaultK10BackupVaultName
return nil, errors.New("EFS vault name is empty")
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you have changed an assumption, I would expect that this would break some unit test in this package ? If not, then that means tests are missing . If so please add them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In k10, efs_kube_test will need to be updated. There is no unit test for NewEFSProvider in kanister however. Let me look into how best test it since the function actually validates creds with AWS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unit test added with 59fb0e5c253bdccf3a688c46c76bcebd7dec7f0f

@shuguet shuguet moved this from To do to Review in progress in Kanister Mar 25, 2022
Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

pkg/blockstorage/awsefs/awsefs_test.go Outdated Show resolved Hide resolved
Kanister automation moved this from Review Required to Reviewer approved Mar 29, 2022
Co-authored-by: Pavan Navarathna <pavan@kasten.io>
@leuyentran
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2022

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 2de85f2 into master Mar 29, 2022
Kanister automation moved this from Reviewer approved to Done Mar 29, 2022
@mergify mergify bot deleted the kanister-efs-backups-remove-reference-to-k10 branch March 29, 2022 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants