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(pubsub): AppSync in China regions #10087

Merged
merged 5 commits into from Dec 8, 2022

Conversation

DiscreteTom
Copy link
Contributor

Fix #10061

Description of changes

Update standardDomainPattern to accept endpoints in China regions.

Issue #, if available

Fix #10061

Description of how you validated changes

Tested using the updated lib and China region AppSync realtime demo. Passed.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

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

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2022

Codecov Report

Merging #10087 (2885401) into main (fc4940b) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #10087      +/-   ##
==========================================
- Coverage   86.16%   86.15%   -0.02%     
==========================================
  Files         196      196              
  Lines       18351    18351              
  Branches     3905     3905              
==========================================
- Hits        15812    15810       -2     
- Misses       2464     2466       +2     
  Partials       75       75              
Impacted Files Coverage Δ
.../src/Providers/AWSAppSyncRealTimeProvider/index.ts 96.06% <100.00%> (ø)
...kages/amazon-cognito-identity-js/src/BigInteger.js 89.31% <0.00%> (-0.41%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@siegerts siegerts added the first-time-contributor The contribution is the first for this user in the repo label Jul 25, 2022
@iartemiev
Copy link
Contributor

@DiscreteTom thank you for opening this PR. Would you mind adding unit tests for this improvement to this file?

@abdallahshaban557
Copy link
Contributor

@DiscreteTom - can you please add a test as Ivan mentioned above?

@@ -40,6 +40,14 @@ describe('AWSAppSyncRealTimeProvider', () => {
);
expect(result).toBe(false);
});

test('Non-custom domain in the amazonaws.com.cn subdomain space returns `false`', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

After manually validating the structure of appsync urls in cn, recommending we add a test, which I've gone ahead and committed.

@stocaaro
Copy link
Contributor

stocaaro commented Dec 7, 2022

@DiscreteTom Thank you so much for this change. I've done my own validation and added a test and will work with the team to get this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor first-time-contributor The contribution is the first for this user in the repo PubSub Related to PubSub category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Amplify GraphQL API (AppSync) not working in China regions.
7 participants