-
Notifications
You must be signed in to change notification settings - Fork 121
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(shorebird_cli): handle multi-dimensional flavors on android #1960
Conversation
packages/shorebird_cli/lib/src/commands/release/release_android_command.dart
Outdated
Show resolved
Hide resolved
import 'package:shorebird_cli/src/command.dart'; | ||
import 'package:shorebird_cli/src/third_party/flutter_tools/lib/flutter_tools.dart'; | ||
|
||
/// Throw when multiple artifacts are found in the build directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will that happen when using Android Studio and building different flavors/configs between multiple runs? I wonder if older .aab's from other configs being in the build dir can cause these failures to happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should never happen as far I can tell, this was more of a "just to be sure" checking that Bryan suggested while we were discussing the solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm also concerned about this case. Have we filed an issue upstream for this bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet. Was focusing on our PR first, will open now that it seems that I addressed all/most of the comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filled: flutter/flutter#147261
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1960 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 176 177 +1
Lines 5520 5560 +40
=========================================
+ Hits 5520 5560 +40
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
Co-authored-by: Felix Angelov <felix@shorebird.dev>
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/commands/release/release_android_command_test.dart
Outdated
Show resolved
Hide resolved
Co-authored-by: Bryan Oltman <bryan@shorebird.dev> Co-authored-by: Felix Angelov <felix@shorebird.dev>
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
Co-authored-by: Bryan Oltman <bryan@shorebird.dev>
packages/shorebird_cli/lib/src/commands/release/release_android_command.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/commands/release/release_android_command.dart
Outdated
Show resolved
Hide resolved
apkFile = shorebirdAndroidArtifacts.findApk( | ||
project: projectRoot, | ||
flavor: flavor, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this the first time, but this reads like it will always fail unless generateApk
is true.
when( | ||
() => shorebirdAndroidArtifacts.findAppBundle( | ||
project: any(named: 'project'), | ||
flavor: any(named: 'flavor'), | ||
), | ||
).thenReturn(File('app-release.aab')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I'd maybe leave a small comment so that's clear
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/commands/release/release_android_command.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/lib/src/shorebird_android_artifacts.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/commands/release/release_android_command_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/commands/release/release_android_command_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/commands/release/release_android_command_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
packages/shorebird_cli/test/src/shorebird_android_artifacts_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just left a bunch of small recommendations
Co-authored-by: Felix Angelov <felix@shorebird.dev>
Description
Our CLI would get confused when using multi dimensional flavors on android. This PR fixes that.
Fixes #1830
Type of Change