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

fix: update pinpoint regions #12379

Merged
merged 13 commits into from
Apr 12, 2023
Merged

fix: update pinpoint regions #12379

merged 13 commits into from
Apr 12, 2023

Conversation

lazpavel
Copy link
Contributor

@lazpavel lazpavel commented Apr 4, 2023

Description of changes

Checklist

  • PR description included
  • yarn test passes
  • migration test added to check that region mapping doesn't affect already created resources

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@lazpavel lazpavel requested a review from a team as a code owner April 4, 2023 15:00
sobolk
sobolk previously approved these changes Apr 4, 2023
Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

Can we confirm this is backwards compatible, specially with customers trying to update their pinpoint resource, will it delete and recreate the resource in the us-east-2 region?

@lazpavel lazpavel marked this pull request as draft April 4, 2023 15:55
@lazpavel
Copy link
Contributor Author

lazpavel commented Apr 4, 2023

@Amplifiyer, from the standpoint of the pinpoint resource, we cannot update the existing one so a push will still keep the pinpoint resource from us-east-1 even after update, there are problems trying to add notifications channels as we use the mapping logic to build the pinpoint client instead of getting the region from the pinpoint resource which in turn fails with Resource not found. Marking it as draft as it requires some design

@Amplifiyer
Copy link
Contributor

We should test and get this merged as well as part of this PR

@lazpavel lazpavel marked this pull request as ready for review April 11, 2023 18:38
sobolk
sobolk previously approved these changes Apr 12, 2023
@@ -11,20 +11,32 @@ import {
$TSMeta,
AmplifyError,
AmplifyFault,
FeatureFlags,
Copy link
Member

Choose a reason for hiding this comment

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

CodeQL says it's unused.

Amplifiyer
Amplifiyer previously approved these changes Apr 12, 2023
Copy link
Contributor

@Amplifiyer Amplifiyer left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +548 to +554
if (httpProxy) {
aws.config.update({
httpOptions: {
agent: proxyAgent(httpProxy),
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a comment here on why http proxy was provided by the user? Any reference to public documentation will help.

@lazpavel lazpavel dismissed stale reviews from Amplifiyer and sobolk via 2efda21 April 12, 2023 18:03
@lazpavel lazpavel merged commit c218dd4 into aws-amplify:dev Apr 12, 2023
2 checks passed
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.

None yet

3 participants