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: raise incorrect post event data #10954

Merged

Conversation

fossamagna
Copy link
Contributor

Description of changes

Using an AmplifyPostPushEventData instance when raised a PostInit event, and an AmplifyPostPushEventData instance when raised a InitEvent event. These use the wrong event data for each other.
Fixed to use the correct event data.

Issue #, if available

Description of how you validated changes

Checklist

  • PR description included
  • yarn test passes

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

@fossamagna fossamagna requested a review from a team as a code owner August 31, 2022 14:26
@danielleadams
Copy link
Contributor

Hey @fossamagna - Thanks for the contribution! I see the function names were changed to match, but what is the context here? Was it breaking anything and was this intentional? Are there any tests we can add to ensure this works? Going to run the CLI test suite now.

@fossamagna
Copy link
Contributor Author

@danielleadams Thank you for your review.
I think that this change is not breaking, because AmplifyPostPushEventData and AmplifyPostPushEventData is not same class, but both class is extended same parent class and both class no have property.
I will add tests to ensure this work.

@fossamagna fossamagna force-pushed the fix/raise-incorrent-event-data branch from b4cf40f to a603926 Compare September 7, 2022 13:55
@codecov-commenter
Copy link

Codecov Report

Merging #10954 (ac62b06) into dev (3c39eba) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev   #10954      +/-   ##
==========================================
+ Coverage   47.82%   48.12%   +0.29%     
==========================================
  Files         669      669              
  Lines       32678    32678              
  Branches     6608     6608              
==========================================
+ Hits        15627    15725      +98     
+ Misses      15419    15329      -90     
+ Partials     1632     1624       -8     
Impacted Files Coverage Δ
packages/amplify-cli/src/execution-manager.ts 54.85% <100.00%> (+38.83%) ⬆️
packages/amplify-cli/src/domain/amplify-event.ts 85.71% <0.00%> (+3.57%) ⬆️
packages/amplify-cli/src/plugin-manager.ts 28.98% <0.00%> (+11.59%) ⬆️
...li/src/domain/amplify-usageData/getUsageDataUrl.ts 100.00% <0.00%> (+12.50%) ⬆️

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

@fossamagna
Copy link
Contributor Author

@danielleadams I added test for execution-manager.ts. It looks like there were nothing failures in the CI, Is there anything blocking to merging this PR?

@hackmajoris
Copy link

Not sure if this issue is related, but I'll leave it here: #11021

@danielleadams danielleadams merged commit 43666d5 into aws-amplify:dev Sep 20, 2022
@fossamagna fossamagna deleted the fix/raise-incorrent-event-data branch September 20, 2022 13:57
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.

5 participants