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

feat: headless command for geo -- map #8403

Merged
merged 8 commits into from Nov 2, 2021

Conversation

AaronZyLee
Copy link
Contributor

@AaronZyLee AaronZyLee commented Oct 8, 2021

Description of changes

Add headless command support for geo category, including

  • add map
  • update map

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

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

@AaronZyLee AaronZyLee requested a review from a team as a code owner October 8, 2021 21:17
@AaronZyLee AaronZyLee marked this pull request as draft October 8, 2021 21:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2021

Codecov Report

Merging #8403 (1edd1aa) into master (39fbe6f) will decrease coverage by 0.04%.
The diff coverage is 20.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8403      +/-   ##
==========================================
- Coverage   57.17%   57.13%   -0.05%     
==========================================
  Files         725      725              
  Lines       40915    40969      +54     
  Branches     8385     8395      +10     
==========================================
+ Hits        23395    23407      +12     
- Misses      16709    16751      +42     
  Partials      811      811              
Impacted Files Coverage Δ
...plify-category-geo/src/provider-controllers/map.ts 40.98% <19.23%> (-2.50%) ⬇️
...ify-category-geo/src/provider-controllers/index.ts 26.00% <20.00%> (-3.17%) ⬇️
packages/amplify-category-geo/src/index.ts 43.10% <25.00%> (-5.84%) ⬇️

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 39fbe6f...1edd1aa. Read the comment docs.

@AaronZyLee AaronZyLee changed the title feat: headless command for geo feat: headless command for geo -- map Oct 11, 2021
@AaronZyLee AaronZyLee marked this pull request as ready for review October 11, 2021 22:30
@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request introduces 1 alert when merging fa8a973 into 7309d96 - view on LGTM.com

new alerts:

  • 1 for Use of returnless function

@lgtm-com
Copy link

lgtm-com bot commented Oct 11, 2021

This pull request fixes 2 alerts when merging a3e736a into 7309d96 - view on LGTM.com

fixed alerts:

  • 2 for Use of returnless function

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2021

This pull request fixes 2 alerts when merging b244723 into 7309d96 - view on LGTM.com

fixed alerts:

  • 2 for Use of returnless function

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2021

This pull request fixes 2 alerts when merging d403cc0 into e190292 - view on LGTM.com

fixed alerts:

  • 2 for Use of returnless function

Copy link
Contributor

@phani-srikar phani-srikar left a comment

Choose a reason for hiding this comment

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

Looks good overall. Things to note before releasing this:

  1. Add remove headless command.
  2. e2e tests to verify that common use-cases work as expected. This can be a separate PR.

packages/amplify-category-geo/src/index.ts Show resolved Hide resolved
"description": "The name of the map that will be created.",
"type": "string"
},
"mapStyle": {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We can probably define this separately like pricingPlan and AccessType.

@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2021

This pull request fixes 2 alerts when merging 9f3725b into 39fbe6f - view on LGTM.com

fixed alerts:

  • 2 for Use of returnless function

@lgtm-com
Copy link

lgtm-com bot commented Oct 27, 2021

This pull request fixes 2 alerts when merging 70d9811 into 39fbe6f - view on LGTM.com

fixed alerts:

  • 2 for Use of returnless function

@lgtm-com
Copy link

lgtm-com bot commented Oct 28, 2021

This pull request introduces 1 alert and fixes 2 when merging 1edd1aa into f72e59b - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

fixed alerts:

  • 2 for Use of returnless function

Copy link
Contributor

@phani-srikar phani-srikar left a comment

Choose a reason for hiding this comment

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

LGTM! remove headless command and e2e tests to be added later.

@AaronZyLee AaronZyLee merged commit 73793b4 into aws-amplify:master Nov 2, 2021
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