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: use existing rather than deleted models #9769

Merged
merged 9 commits into from
Feb 25, 2022

Conversation

marcvberg
Copy link
Contributor

Description of changes

When looking for missing function dependencies on API, a model diff against current backend doesn't work if the model was both added, function added with dependency, and model subsequently removed prior to push. This addresses that by looking to see if functions depend on models not in the backend, rather than diffing against cloud backend

Issue #, if available

#9553

Description of how you validated changes

Running unit tests, making an application and testing using the original reproduction steps

Checklist

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

When looking for missing function dependencies on API, a model diff against current backend doesn't work if the model was both added, function added with dependency, and model subsequently removed prior to push. This addresses that by looking to see if functions depend on models not in the backend, rather than diffing against cloud backend
@lgtm-com
Copy link

lgtm-com bot commented Feb 15, 2022

This pull request introduces 1 alert when merging 88dbf9c into f1af5bb - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #9769 (e70366e) into master (943c7ed) will increase coverage by 0.00%.
The diff coverage is 65.90%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9769   +/-   ##
=======================================
  Coverage   52.99%   52.99%           
=======================================
  Files         830      830           
  Lines       45993    46022   +29     
  Branches     9823     9837   +14     
=======================================
+ Hits        24375    24391   +16     
- Misses      19600    19613   +13     
  Partials     2018     2018           
Impacted Files Coverage Δ
...y-provider-awscloudformation/src/push-resources.ts 17.30% <0.00%> (ø)
...udformation/src/utils/remove-dependent-function.ts 10.00% <0.00%> (+1.30%) ⬆️
.../awscloudformation/utils/loadFunctionParameters.ts 84.61% <33.33%> (-15.39%) ⬇️
...ls/awscloudformation/utils/getDependentFunction.ts 83.33% <70.58%> (-16.67%) ⬇️
packages/amplify-category-function/src/index.ts 47.70% <100.00%> (ø)
...cloudformation/utils/updateDependentFunctionCfn.ts 72.22% <100.00%> (-11.46%) ⬇️

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 943c7ed...e70366e. Read the comment docs.

@marcvberg marcvberg marked this pull request as ready for review February 17, 2022 18:52
@marcvberg marcvberg requested a review from a team as a code owner February 17, 2022 18:52
@lgtm-com
Copy link

lgtm-com bot commented Feb 17, 2022

This pull request introduces 1 alert when merging 247aa25 into 2bd4172 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@marcvberg marcvberg linked an issue Feb 17, 2022 that may be closed by this pull request
4 tasks
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 changed the title Fix: Use existing rather than deleted models fix: use existing rather than deleted models Feb 17, 2022
marcvberg and others added 2 commits February 17, 2022 16:56
Co-authored-by: John Hockett <jhockett@users.noreply.github.com>
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Left some small comments. Mostly LGTM. Please run the linter against these changes.

@sachscode sachscode merged commit c4b7487 into aws-amplify:master Feb 25, 2022
sachscode added a commit that referenced this pull request Mar 1, 2022
sachscode added a commit that referenced this pull request Mar 1, 2022
@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label Mar 7, 2022
@github-actions
Copy link

github-actions bot commented Mar 7, 2022

👋 Hi, this pull request was referenced in the v7.6.23 release!

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

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.

StreamARN to resource is not cleaned up after type was removed from schema
5 participants