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.

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

chore: fix aft version-bump #5436

Merged
merged 2 commits into from
Sep 12, 2024
Merged

chore: fix aft version-bump #5436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c91815a
fix: properly handle component propagation
Jordan-Nelson Sep 11, 2024
7e254e5
chore: add test for multi package update with breaking common package
Jordan-Nelson Sep 11, 2024
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
20 changes: 7 additions & 13 deletions packages/aft/lib/src/repo.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,6 @@ class Repo {
required VersionBumpType type,
required bool Function(PackageInfo) canBump,
required bool includeInChangelog,
bool? propagateToComponent,
}) {
logger.verbose('bumpVersion ${package.name} $commit');
final componentName = aftConfig.componentForPackage(package.name);
Expand All @@ -317,7 +316,7 @@ class Repo {
],
(version) => version,
)!;
propagateToComponent ??= component != null &&
final propagateToComponent = component != null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Can you provide context around the purpose of this flag? How is it set?

Copy link
Member Author

Choose a reason for hiding this comment

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

aft defines components in the root pubspec.yaml. Below is an example of the "Amplify Flutter" component which includes all amplify packages intended for direct consumption in a Flutter app.

amplify-flutter/pubspec.yaml

Lines 69 to 82 in f825f86

components:
- name: Amplify Flutter
summary: amplify_flutter
packages:
- amplify_flutter
- amplify_core
- amplify_datastore
- amplify_datastore_plugin_interface
- amplify_analytics_pinpoint
- amplify_api
- amplify_auth_cognito
- amplify_storage_s3
- amplify_push_notifications
- amplify_push_notifications_pinpoint

By default, if a package has a minor/major bump, that bump should propagate to all other packages in the components. For example, if amplify_auth_cognito goes from 2.3.1 to 2.4.0, all packages in that component have to have their version bumped to 2.4.0.

Components can override the default behavior by specifying propagate: "major" | "minor" | "all" | "none" in the pubpsec.yaml file. The "Amplify Dart" component has this override. When amplify_auth_cognito_dart has a minor version bump, other packages in the group do not need to also be bumped.

amplify-flutter/pubspec.yaml

Lines 83 to 90 in f825f86

- name: Amplify Dart
summary: amplify_core
propagate: none
packages:
- amplify_analytics_pinpoint_dart
- amplify_api_dart
- amplify_auth_cognito_dart
- amplify_storage_s3_dart

The component.propagate.propagateToComponent() fn determines if a change needs to propagate based on the component config in the root pubpsec file and the version update.

Prior to this PR, this fn accepted an optional propagateToComponent flag. I do not know what the intended purpose of this was, but it was preventing propagation from occurring more than 1 level deep. This is problematic. See the PR description for an example of an issue this caused.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes more sense now, thanks for providing those references!

component.propagate.propagateToComponent(
currentVersion,
newVersion,
Expand Down Expand Up @@ -359,11 +358,13 @@ class Repo {
pkg.pubspecInfo.pubspec.dependencies.keys.contains(package.name) ||
pkg.pubspecInfo.pubspec.devDependencies.keys.contains(package.name),
);
if (commit.isBreakingChange) {
// Back-propagate to all dependent packages for breaking changes.
if (commit.isBreakingChange || propagateToComponent) {
// Back-propagate to all dependent packages for breaking changes or
// changes that need to propagate to a component.
//
// Since we set semantic version constraints, only a breaking change
// in a direct dependency necessitates a version bump.
// Since we set semantic version constraints, only a breaking change in
// a direct dependency or a change that requires propagation
// necessitates a version bump.
logger.verbose(
'Breaking change. Performing dfs on dependent packages...',
);
Expand All @@ -380,12 +381,6 @@ class Repo {
}
updateConstraint(package, dependent);
}
} else if (type == VersionBumpType.nonBreaking) {
// For non-breaking changes, we still need to update all constraints
// since we "pin" to minor versions.
for (final dependent in packageDependents) {
updateConstraint(package, dependent);
}
Equartey marked this conversation as resolved.
Show resolved Hide resolved
}

// Propagate to all component packages.
Expand All @@ -408,7 +403,6 @@ class Repo {
type: type,
canBump: canBump,
includeInChangelog: false,
propagateToComponent: false,
);
},
);
Expand Down
65 changes: 28 additions & 37 deletions packages/aft/test/version_bump/data/snapshots/multi_package_update.diff
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading