Skip to content

chore(amplify_analytics): switch to federated plugins#1378

Merged
fjnoyp merged 12 commits intoaws-amplify:mainfrom
fjnoyp:federated/analytics
Mar 11, 2022
Merged

chore(amplify_analytics): switch to federated plugins#1378
fjnoyp merged 12 commits intoaws-amplify:mainfrom
fjnoyp:federated/analytics

Conversation

@fjnoyp
Copy link
Contributor

@fjnoyp fjnoyp commented Feb 18, 2022

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@fjnoyp fjnoyp requested a review from haverchuck February 18, 2022 23:26
@fjnoyp fjnoyp requested a review from a team as a code owner February 18, 2022 23:26
@haverchuck
Copy link
Contributor

haverchuck commented Feb 28, 2022

I'm not sure why so many of these files are showing up as new. Did you use git mv?

Update: actually it looks like you may have committed some files from the older analyics directory (i.e. outside of the _plugin directory)? Potentially some auto generated stuff?

@fjnoyp fjnoyp requested a review from haverchuck March 2, 2022 21:22
@@ -9,6 +9,8 @@ environment:

dependencies:
Copy link
Member

Choose a reason for hiding this comment

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

I can't leave a comment on the correct line, but on line 4 the homepage will need to be updated.

- https://github.com/aws-amplify/amplify-flutter/tree/main/packages/amplify_analytics_pinpoint
+ https://github.com/aws-amplify/amplify-flutter/tree/main/packages/analytics/amplify_analytics_pinpoint

This will unfortunately make pub analysis fail temporarily because the new URL will return a 404. Once the PR is merged, the URL will return a 200 and pub analysis will pass. The is the change @haverchuck made in #1424 for auth.

We will just have to merge this PR with one failing CI step. We should confirm that the homepage change is the only reason it is failing though.

Copy link
Member

Choose a reason for hiding this comment

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

This comment applies to the storage PR as well by the way.

Both analytics and storage are scoring 110/120 on their pub score, which is passing in CI. With the homepage change, they will start scoring 100/120, which will fail in CI. If they score below 100, then we should figure out why before merging.

We should also keep an eye on the post-merge CI run to confirm it passes after this merges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks sounds good, updated homepage field in pubspec.yaml

@haverchuck
Copy link
Contributor

@fjnoyp I think rebasing will help these analysis issues.

@fjnoyp fjnoyp merged commit 9fbc55d into aws-amplify:main Mar 11, 2022
@fjnoyp fjnoyp deleted the federated/analytics branch March 11, 2022 19:54
HuiSF pushed a commit to HuiSF/amplify-flutter that referenced this pull request Mar 11, 2022
* chore(amplify_analytics): switch to federated plugins
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.

3 participants