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: (Geo) Kebab case to Camel case config props #12921

Closed

Conversation

nadetastic
Copy link
Contributor

@nadetastic nadetastic commented Jan 29, 2024

Description of changes

Issue #, if available

#12870
#12752

Description of how you validated changes

  • Tested with local build

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 Jan 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (278776f) 88.02% compared to head (38164f8) 87.28%.
Report is 5 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12921      +/-   ##
==========================================
- Coverage   88.02%   87.28%   -0.74%     
==========================================
  Files         663      635      -28     
  Lines       18141    13446    -4695     
  Branches     3635     2462    -1173     
==========================================
- Hits        15969    11737    -4232     
+ Misses       2172     1709     -463     
Flag Coverage Δ
adapter-nextjs 98.64% <ø> (ø)
analytics 62.75% <ø> (ø)
api ∅ <ø> (∅)
api-graphql 87.71% <ø> (ø)
api-rest 95.95% <ø> (-0.33%) ⬇️
auth 86.78% <ø> (ø)
aws-amplify 100.00% <ø> (ø)
core 87.72% <ø> (-0.31%) ⬇️
datastore ?
datastore-storage-adapter 88.13% <ø> (ø)
geo 90.12% <ø> (ø)
interactions 96.10% <ø> (ø)
notifications 90.35% <ø> (ø)
predictions 86.00% <ø> (ø)
pubsub 94.70% <ø> (ø)
rtn-push-notification 100.00% <ø> (ø)
storage 91.28% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nadetastic nadetastic marked this pull request as ready for review February 1, 2024 19:19
@nadetastic nadetastic requested review from a team as code owners February 1, 2024 19:19
cwomack
cwomack previously approved these changes Feb 1, 2024
Copy link
Contributor

@cwomack cwomack left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@calebpollman calebpollman left a comment

Choose a reason for hiding this comment

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

Think that the changes in AmazonLocationServiceProvider are non-breaking, but the fact that this is only an issue for Gen2 makes me concerned that there is something off with the returned shape of Geo in parseAWSExports as existing apps using v6 do not exhibit the issue that this PR is addressing. Before approving:

  • Need to understand why non-Gen2 customers are not facing this issue
  • Need to see a successful run of the UI e2e tests with this change

packages/geo/__tests__/testData.ts Outdated Show resolved Hide resolved
kvramyasri7
kvramyasri7 previously approved these changes Feb 2, 2024
Copy link
Contributor

@kvramyasri7 kvramyasri7 left a comment

Choose a reason for hiding this comment

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

Nice

@nadetastic
Copy link
Contributor Author

Think that the changes in AmazonLocationServiceProvider are non-breaking, but the fact that this is only an issue for Gen2 makes me concerned that there is something off with the returned shape of Geo in parseAWSExports as existing apps using v6 do not exhibit the issue that this PR is addressing. Before approving:

  • Need to understand why non-Gen2 customers are not facing this issue
  • Need to see a successful run of the UI e2e tests with this change

@calebpollman I wouldn’t classify it as a Gen2 vs Gen1 thing but more of Legacy vs Resource config issue:

  • Only happens when the Geo resources are manually added as ResourceConfig (camelCase) and does not happen if added as Legacy config (kebab_case)
  • The library (or really AmazonLocationServiceProvider) is expecting it to be in kebab_case (aka legacy config) however it is in camelCase (aka Resource config)
  • An important note (why it works when parseExports is used on LegacyConfig) - there are two object created when legacy config is used - one with camelCase and a copy in kebab_case, however, if Resource config is used, the legacy config object is not present which results in the error

Working on the the UI e2e tests

@calebpollman
Copy link
Contributor

@calebpollman I wouldn’t classify it as a Gen2 vs Gen1 thing but more of Legacy vs Resource config issue:

  • Only happens when the Geo resources are manually added as ResourceConfig (camelCase) and does not happen if added as Legacy config (kebab_case)
  • The library (or really AmazonLocationServiceProvider) is expecting it to be in kebab_case (aka legacy config) however it is in camelCase (aka Resource config)
  • An important note (why it works when parseExports is used on LegacyConfig) - there are two object created when legacy config is used - one with camelCase and a copy in kebab_case, however, if Resource config is used, the legacy config object is not present which results in the error

Working on the the UI e2e tests

From what i am reading here, the fact that parseAWSExports returns both camelCase and kebab_case objects is part of the root issue since the return does not match the expected shape of the ResourcesConfig, and should be addressed as a part of this PR to align handling of projects with hand rolled ResourcesConfig and legacy configs.

Once the handling of the Geo config parsing is fixed, the updates here, here and here to the differing inputs should be fine, but we should make sure that we do need to include lookup against the search_indices as well to ensure backwards compatibility.

@nadetastic nadetastic requested a review from a team as a code owner March 13, 2024 21:49
@nadetastic nadetastic changed the title fix: Kebab case to Camel case config props fix: (Geo) Kebab case to Camel case config props Mar 25, 2024
@nadetastic
Copy link
Contributor Author

Updated PR to address the issue where parseAWSExports was returning both searchIndices and search_indices

@AllanZhengYP
Copy link
Contributor

Close this request since it has been replaced by #13303 and released

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

7 participants