Conversation
|
S3SnapshotRestoreTestSuite TearDownSuite TestSuites above were modified. TestSuites below use modified code from this PR. |
navsalehi
requested review from
Copilot,
markusewalker and
susesgartner
and removed request for
Copilot
| s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" | ||
| ) | ||
|
|
||
| // builds AWS config using static credentials and region |
There was a problem hiding this comment.
nit: generally speaking its standard to add the function name to the string so that would mean this would look like
// awsS3Config builds AWS config using static credentials and region
| } | ||
|
|
||
| func (s *S3SnapshotRestoreTestSuite) TearDownSuite() { | ||
| if s.createdTestBucket && s.s3BucketName != "" { |
There was a problem hiding this comment.
I'm a bit curious, what would happen if the test failed before creating the S3 bucket. Would this throw a cleanup error and cause the rest of cleanup to not run?
markusewalker
left a comment
markusewalker
left a comment
There was a problem hiding this comment.
Left comments that apply to both K3s and RKE2. Once addressed, let's test again to ensure all is good. Overall, nice job!
| @@ -3,22 +3,28 @@ | |||
| package k3s | |||
There was a problem hiding this comment.
We will want both k3s and rke2 in our recurring tests, so please be sure to add //go:build validation || recurring at the beginning of both files.
| s.session.Cleanup() | ||
| } | ||
|
|
||
| type awsCredentialsConfig struct { |
There was a problem hiding this comment.
I do not think this is necessary. clusterConfig parameter has all of these defined and this test is assumed to be ran with AWS. So this should just be removed.
| client *rancher.Client | ||
| cattleConfig map[string]any | ||
| cluster *v1.SteveAPIObject | ||
| s3BucketName string |
There was a problem hiding this comment.
The s3 params you added in the struct can be removed. You would do this if you reference in a test function, but it doesn't seem that you're doing that in this PR. Since it's all seemingly in the SetupSuite function, you would would just remove these and remove the s. when you do call it in this function.
| awsCredsConfig := new(awsCredentialsConfig) | ||
| operations.LoadObjectFromMap("awsCredentials", s.cattleConfig, awsCredsConfig) | ||
|
|
||
| s.awsAccessKey = awsCredsConfig.AccessKey |
There was a problem hiding this comment.
These two lines should be removed; see other comments as to why.
| }, | ||
| } | ||
|
|
||
| logrus.Infof("Provisioning K3S cluster with S3 snapshot config. bucket=%s region=%s endpoint=%s credential=%s", s.s3BucketName, s.s3Region, s.s3Endpoint, s.s3CloudCredName) |
There was a problem hiding this comment.
You can just delete this log, not really necessary.
3 participants
This PR adds S3 bucket handling to the snapshot restore tests and introduces reusable helpers (CreateS3Bucket and DeleteS3Bucket) under actions/etcdsnapshot to avoid duplication.
Both K3S and RKE2 tests now create a bucket dynamically, configure ETCD to use S3, validate snapshot/restore, and clean up the bucket during teardown.