Skip to content

Conversation

@rajrajhans
Copy link
Contributor

@rajrajhans rajrajhans commented Apr 10, 2021

Description of changes

Optional chaining operator is used in packages/amplify-provider-awscloudformation/src/system-config-manager.js. However, since it is a JS file, and not a TS file, it isn't being processed by Babel and so some tests are failing ->

image

This PR converts packages/amplify-provider-awscloudformation/src/system-config-manager.js to a .ts file (as suggested by @cjihrig)

Issue #, if available

Description of how you validated changes

yarn test

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.

@rajrajhans rajrajhans requested a review from a team as a code owner April 10, 2021 16:29
@cjihrig
Copy link
Contributor

cjihrig commented Apr 10, 2021

Instead of adding more complexity, can we just change system-config-manager.js to a TypeScript file? Something like this commit, for example.

@rajrajhans
Copy link
Contributor Author

Instead of adding more complexity, can we just change system-config-manager.js to a TypeScript file? Something like this commit, for example.

Oh, I didn't know that changing to .ts will fix it. Thanks.

@codecov
Copy link

codecov bot commented Apr 10, 2021

Codecov Report

Merging #7053 (8c56d64) into master (1bb30e8) will increase coverage by 0.11%.
The diff coverage is 72.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7053      +/-   ##
==========================================
+ Coverage   56.31%   56.43%   +0.11%     
==========================================
  Files         445      445              
  Lines       21806    21859      +53     
  Branches     4363     4373      +10     
==========================================
+ Hits        12280    12336      +56     
- Misses       8711     8745      +34     
+ Partials      815      778      -37     
Impacted Files Coverage Δ
packages/amplify-cli/src/app-config/config.ts 100.00% <ø> (ø)
...fy-cli/src/domain/amplify-usageData/NoUsageData.ts 14.28% <ø> (ø)
packages/amplify-cli/src/initialize-env.ts 10.93% <0.00%> (-0.18%) ⬇️
...lify-provider-awscloudformation/src/aws-regions.js 100.00% <ø> (ø)
...der-awscloudformation/src/configuration-manager.ts 5.36% <10.00%> (-0.08%) ⬇️
packages/amplify-cli/src/index.ts 42.10% <50.00%> (ø)
...ages/graphql-key-transformer/src/KeyTransformer.ts 93.11% <50.00%> (+0.03%) ⬆️
packages/amplify-cli/src/context-manager.ts 78.12% <60.00%> (-16.62%) ⬇️
...der-awscloudformation/src/system-config-manager.ts 23.35% <95.00%> (ø)
...sync-simulator/src/schema/appsync-scalars/index.ts 47.31% <100.00%> (+2.25%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebdaa59...8c56d64. Read the comment docs.

@rajrajhans rajrajhans changed the title fix(amplify-provider-awscloudformation): add @babel/plugin-proposal-optional-chaining to fix failing tests fix(amplify-provider-awscloudformation): fix tests failing due to system-config-manager.js Apr 10, 2021
Copy link
Contributor

@jhockett jhockett left a comment

Choose a reason for hiding this comment

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

LGTM!

@jhockett jhockett merged commit 07525b3 into aws-amplify:master Apr 10, 2021
edwardfoyle pushed a commit to edwardfoyle/amplify-cli that referenced this pull request Apr 12, 2021
@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Apr 14, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v4.48.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.48.0.

akshbhu pushed a commit that referenced this pull request Jun 10, 2021
* fix(amplify-provider-awscloudformation): fix tests failing due to system-config-manager.js (#7053)

* feat: s3 sse by default

* chore: fix init push issue

* chore: cleanup

* test: whole lotta tests

* test: update nondeterministic test

* fix: serialize modifiers and improve test error handling

* fix: add parameterization to ResourceModifier

* fix: add type to sig

* test: update test with new modifier structure

* test: fix test

* feat: add permission boundary to IAM roles

* fix: update iam role modifier

* test: add e2e test for perm bound

* test: add unit tests for perm bound modifier

* fix: fix regex

* feat: switch to env-specific config

* chore: dumping env perm bound changes

* feat: fixup env-specific config and add headless support

* chore: cleaning up things

* test: more unit tests and e2e test

* test: small test tweaks

* chore: reverting some unintentional linting changes

* fix: add update to env help text

* test: add mock

* chore: address PR comments

* chore: use module var instead of global var

* chore: rename permission boundary -> permissions boundary

* fix: merge tpi instead of overwrite

* chore: remove newline

* fix: load creds for new env when checking policy

* fix: test fixes

* test: fix unit tests

* test: fix profile selection

* fix: change permissions boundary success text

Co-authored-by: Raj Rajhans <me@rajrajhans.com>
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v5.0.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v5.0.0.

cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
…(ref aws-amplify#4618)

* fix(amplify-provider-awscloudformation): fix tests failing due to system-config-manager.js (aws-amplify#7053)

* feat: s3 sse by default

* chore: fix init push issue

* chore: cleanup

* test: whole lotta tests

* test: update nondeterministic test

* fix: serialize modifiers and improve test error handling

* fix: add parameterization to ResourceModifier

* fix: add type to sig

* test: update test with new modifier structure

* test: fix test

* feat: add permission boundary to IAM roles

* fix: update iam role modifier

* test: add e2e test for perm bound

* test: add unit tests for perm bound modifier

* fix: fix regex

* feat: switch to env-specific config

* chore: dumping env perm bound changes

* feat: fixup env-specific config and add headless support

* chore: cleaning up things

* test: more unit tests and e2e test

* test: small test tweaks

* chore: reverting some unintentional linting changes

* fix: add update to env help text

* test: add mock

* chore: address PR comments

* chore: use module var instead of global var

* chore: rename permission boundary -> permissions boundary

* fix: merge tpi instead of overwrite

* chore: remove newline

* fix: load creds for new env when checking policy

* fix: test fixes

* test: fix unit tests

* test: fix profile selection

* fix: change permissions boundary success text

Co-authored-by: Raj Rajhans <me@rajrajhans.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

referenced-in-release Issues referenced in a published release changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants