-
Notifications
You must be signed in to change notification settings - Fork 81
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
chore: Improve AWSS3 SigV4A integration tests to use preexisting MRAP if applicable. #1370
Conversation
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.
See questions. I suspect changes are required but have a look.
accountId: accountId, | ||
name: mrapName | ||
)).accessPoint?.status | ||
} while status == .creating |
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.
Do we need to have some sort of timeout mechanism for this repeat
loop?
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 a simple polling limit of 20 with poll count incrementing by 1 each time.
// Wait until MRAP creation finishes | ||
var status: S3ControlClientTypes.MultiRegionAccessPointStatus? = .creating | ||
repeat { | ||
let seconds = 20.0 |
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.
nit: use TimeInterval
type for a number of seconds
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.
Changed as suggested.
// MRAP must be deleted before deleting the linked bucket | ||
try await deleteMRAP() | ||
// Empty & delete the bucket | ||
try await super.tearDown() |
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.
This test class inherits S3XCTestCase
, which does have a tear-down... we do call super.setUp()
at least in some cases, which creates a test bucket... do we need to call super.tearDown()
as well?
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.
The super.tearDown()
deletes the bucket that was created in super.setUp()
, but since we're keeping the MRAP, we can't delete the bucket. To delete the bucket, we have to delete MRAP first. Otherwise it would result in an error.
Issue #
#1371
Description of changes
New/existing dependencies impact assessment, if applicable
Conventional Commits
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.