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

Upgrade MP and adding tests for S3 Express #90

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Conversation

dlakhaws
Copy link
Contributor

@dlakhaws dlakhaws commented Dec 1, 2023

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dlakhaws dlakhaws force-pushed the dl-express-testing branch 5 times, most recently from 17898e0 to be66e1c Compare December 1, 2023 04:20
@dlakhaws dlakhaws changed the title Upgrading MP and adding tests for S3 Express Upgrade MP and adding tests for S3 Express Dec 1, 2023
@dlakhaws dlakhaws force-pushed the dl-express-testing branch 8 times, most recently from 7546a01 to dc41d5d Compare December 1, 2023 20:54
jjkr
jjkr previously approved these changes Dec 1, 2023
// assume us-east-1 since that's where our integration tests currently do their work
// https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-express-networking.html
regionAz := "use1-az4"
if BucketRegion == "us-west-2" {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this won't work for regions that aren't us-east-1 or us-west-2? We should at least track a task that will fix this for other regions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea I actually took a brief look into this but there's no programmatic way to see where S3 Express is supported, and right now the only NA regions it supports is us-east-1 and us-west-2, but agreed something to look into in the future

@@ -61,7 +61,7 @@ jobs:
needs: build
strategy:
matrix:
cluster-type: ["kops", "eksctl"]
cluster-type: ["eksctl", "kops"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the swap here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unintentional, I had removed it in a previous commit to test kops only and then added it back in but in reverse order

@dlakhaws dlakhaws merged commit 15277b0 into main Dec 4, 2023
7 checks passed
@dlakhaws dlakhaws deleted the dl-express-testing branch December 4, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants