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

Shall we consider setting the default region as us-east-1? #41

Open
jizusun opened this issue Jun 13, 2023 · 3 comments
Open

Shall we consider setting the default region as us-east-1? #41

jizusun opened this issue Jun 13, 2023 · 3 comments

Comments

@jizusun
Copy link

jizusun commented Jun 13, 2023

This what backstage does.

https://github.com/backstage/backstage/blob/999936b3915d807206f833f44c8397775780a22e/packages/backend-common/src/reading/AwsS3UrlReader.ts#L50

export const DEFAULT_REGION = 'us-east-1';

Shall we consider this?

With this approach, the region is not a breaking change any more.

@heyLu
Copy link
Member

heyLu commented Jun 13, 2023

I suppose following what Backstage does might be a good idea, although it does feel like a bit of a workaround that was just added to work with the new aws-sdk version.

We don't use AWS S3 internally, thus the region value does not actually matter to us, so do feel free to open a PR that adds us-east-1 as a default.

With this approach, the region is not a breaking change any more.

I don't quite understand this, what do you mean by the region being a breaking change?

@jizusun
Copy link
Author

jizusun commented Jun 19, 2023

I suppose following what Backstage does might be a good idea, although it does feel like a bit of a workaround that was just added to work with the new aws-sdk version.

We don't use AWS S3 internally, thus the region value does not actually matter to us, so do feel free to open a PR that adds us-east-1 as a default.

With this approach, the region is not a breaking change any more.

I don't quite understand this, what do you mean by the region being a breaking change?

Because when I check the release page https://github.com/spreadshirt/backstage-plugin-s3/releases/tag/%40spreadshirt%2Fbackstage-plugin-s3-viewer-common%400.3.0
It says

e83665c: BREAKING: Migrated to AWS-SDK version 3. Now the platform config requires a new field called region. If not present, the plugin will fail on startup.

So it's a breaking change, isn't it?

@heyLu
Copy link
Member

heyLu commented Jun 19, 2023

Ah yes, that makes sense. :) If a default is introduced then there will still have been a breaking change, but not if you skip the version with the required region parameter and go directly to one with a new default.

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

No branches or pull requests

2 participants