Skip to content

chore: add pub analysis to CI #1408

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.

Sign up for GitHub

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

Merged
Jordan-Nelson merged 23 commits into from
Mar 1, 2022
Merged

chore: add pub analysis to CI #1408

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
96bbd52
add pana to ci
Jordan-Nelson Feb 25, 2022
86fc629
add missing colon
Jordan-Nelson Feb 25, 2022
6563b70
add install_melos step
Jordan-Nelson Feb 25, 2022
d4c43a1
move scope arg
Jordan-Nelson Feb 25, 2022
a674a6e
move exec command to config
Jordan-Nelson Feb 25, 2022
a820264
remove pana from melos
Jordan-Nelson Feb 25, 2022
a7667b9
add remaining packages
Jordan-Nelson Feb 25, 2022
ca448c3
address issue in authenticator package
Jordan-Nelson Feb 25, 2022
2dfa627
disable failing packages
Jordan-Nelson Feb 25, 2022
89f04da
revert change to melos.yaml
Jordan-Nelson Feb 25, 2022
aea3f8b
add threshold
Jordan-Nelson Feb 28, 2022
e7db9e5
add quotes
Jordan-Nelson Feb 28, 2022
0c843b7
escape quotes
Jordan-Nelson Feb 28, 2022
a384b7c
escape << >>
Jordan-Nelson Feb 28, 2022
6c7c6bb
escape <<<
Jordan-Nelson Feb 28, 2022
789b932
update variable parsing
Jordan-Nelson Feb 28, 2022
f9e701b
update parsing
Jordan-Nelson Feb 28, 2022
025289b
remove trailing quote
Jordan-Nelson Feb 28, 2022
966f1b8
add remaining packages
Jordan-Nelson Feb 28, 2022
260bd02
update variable name, add desc
Jordan-Nelson Feb 28, 2022
fa7acfb
adjust thresholds
Jordan-Nelson Feb 28, 2022
6f88729
Merge branch 'main' into ci/pana
Jordan-Nelson Feb 28, 2022
8d5731c
decrease api threshold
Jordan-Nelson Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ commands:
command: |
flutter pub global activate melos 1.3.0
melos bootstrap
activate_pana:
steps:
- run:
name: Install and set up pana
command: |
flutter pub global activate pana
install_tuneup:
steps:
- run:
Expand All @@ -62,6 +68,20 @@ jobs:
- run:
name: Analyze Dart/Flutter Code
command: melos run analyze --no-select

pub_analysis:
executor: docker-executor
parameters:
plugin:
type: string
steps:
- install_flutter
- checkout
- install_melos
- activate_pana
- run:
name: Run pub analysis and fail if the score is below the max score
command: melos exec -c 1 --fail-fast --scope="<< parameters.plugin >>" -- pana --no-warning --exit-code-threshold 0 .

unit_test_flutter:
executor: docker-executor
Expand Down Expand Up @@ -181,6 +201,21 @@ workflows:
jobs:
- format_flutter
- analyze_flutter
- pub_analysis:
matrix:
parameters:
plugin:
[
"amplify_authenticator",
# TODO: enable other packages once pub score is addressed

@Jordan-Nelson Jordan-Nelson Feb 25, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

amplify_authenticator is the only package with a max score right now.

Note: 120 is considered a max score for packages that don't support web, even though adding web support would bump the score to 130. It is still considered a "max" by pana,

@ragingsquirrel3 ragingsquirrel3 Feb 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems like the other packages lose points for, among other things, a dev dependency on unpublished package amplify_test. Any ideas for how to resolve that? Without fixing that, it seems we will either a) have to use only in authenticator b) change the exit-code-threshold to 10 (I think?) to basically ignore that loss of points.

@Jordan-Nelson Jordan-Nelson Feb 25, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't know off the top of my head. If we cannot figure out how to address that, we could set the threshold to 10, or set different threshold per package.

@ragingsquirrel3 ragingsquirrel3 Feb 26, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok, makes sense. For this PR, I would either

a) merge it like this and figure it out when we add other plugins
b) do this now for any packages that can be published with threshold of 10 or 0.

@Jordan-Nelson Jordan-Nelson Feb 28, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I updated the PR to use a different threshold for each plugin. The one quirk with this is that the threshold is now in the CI job name. That means we will have to change the list of required jobs each time we adjust the threshold. I think thats fine though as it shouldn't be changed frequently.

# "amplify_analytics_pinpoint",
# "amplify_api",
# "amplify_auth_cognito",
# "amplify_core",
# "amplify_datastore",
# "amplify_flutter",
# "amplify_storage_s3",
]
- unit_test_flutter:
matrix:
parameters:
Expand Down
1 change: 0 additions & 1 deletion packages/amplify_authenticator/lib/src/services/amplify_auth_service.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import 'dart:async';
import 'package:amplify_auth_cognito/amplify_auth_cognito.dart';
import 'package:amplify_flutter/amplify_flutter.dart';
import 'package:collection/collection.dart';
import 'package:collection/src/iterable_extensions.dart';

@Jordan-Nelson Jordan-Nelson Feb 25, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removing this was needed to get the authenticator to a max pub score

@haverchuck haverchuck Feb 25, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm assuming it was superfluous and we had access to the extensions via collection.dart?

@Jordan-Nelson Jordan-Nelson Feb 25, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct


abstract class AuthService {
Future<SignInResult> signIn(String username, String password);
Expand Down