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

feat(aft): Version bump command #2068

Merged
merged 44 commits into from
Jan 10, 2023
Merged

Conversation

dnys1
Copy link
Contributor

@dnys1 dnys1 commented Aug 26, 2022

Introduces a new version-bump command which bumps versions throughout the repo according to Amplify semantics, and updates changelogs accordingly.

To bump the version: aft version-bump
To bump the version and see why certain changes were made: aft version-bump -v (usually helps to redirect or tee to a file)

The Algorithm

The version-bump command uses Git history + conventional commit formatting to determine a suitable next version for a package along with the required changes for depending packages.

  1. Let packages be the set of all packages in the repo which are publishable to pub.dev.
  2. For every package P in packages:
    1. Let component be the component of P, if any.
    2. Let baseRef be the commit of the last release of P.
    3. Let headRef be the releaseable commit of P (defaults to HEAD).
    4. Let history be the list of git commits in the range baseRef..headRef which affected P, i.e. those commits which included changes to files in P.
    5. Let nextVersion = currentVersion.
    6. For each commit in history:
      1. If commit is a version bump (i.e. chore(version)), ignore it.
      2. If commit is a merge commit, update dependencies based on the packages changed by the commit.
        1. The thinking here is that PRs should either be squashed into a single commit or merged as a set of independent commits capped off by a merge commit. The independent commits are isolated changes which are used to update changelogs and bump versions. The merge commit is then used solely for associating previous commits and updating constraints accordingly.
      3. If commit is a breaking change (i.e. feat(auth)!), set bumpType = breaking.
        1. else if commit's type is feat, set bumpType = nonBreaking.
        2. else, set bumpType = patch.
      4. If commit is a noteworthy change (scope is one of feat, fix, bug, perf, or revert or it's a breaking change), set includeInChangelog = true.
      5. Let proposedVersion = currentVersion.bump(bumpType)
      6. Let nextVersion = max(nextVersion, proposedVersion)
      7. If nextVersion > currentVersion:
        1. Update pubspec.yaml, set version = nextVersion
        2. If includeInChangelog:
          1. Update CHANGELOG.md with an entry for commit.
        3. If bumpType == breaking:
          1. For every package Q which directly depends on P:
            1. Bump the version of Q with bumpType = patch and includeInChangelog = false.
            2. Update Q's constraint on P.
        4. If bumpType == breaking or bumpType == nonBreaking and component != null:
          1. For every package Q in component:
            1. Bump the version of Q with the same bumpType as P and includeInChangelog = false.
      8. If component has a summary package:
        1. Update CHANGELOG.md in the summary package with commit.
      9. For every package Q which was affected by commit:
        1. Update Q's constraint on P using nextVersion.

@dnys1 dnys1 requested a review from a team as a code owner August 26, 2022 21:20
@dnys1 dnys1 marked this pull request as draft August 26, 2022 22:12
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2022

Codecov Report

Merging #2068 (2cc6576) into next (db69cce) will increase coverage by 8.79%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             next    #2068      +/-   ##
==========================================
+ Coverage   47.44%   56.24%   +8.79%     
==========================================
  Files          98      115      +17     
  Lines        5705     6989    +1284     
==========================================
+ Hits         2707     3931    +1224     
- Misses       2998     3058      +60     
Flag Coverage Δ
android-unit-tests ∅ <ø> (∅)
flutter-unit-tests 47.44% <ø> (ø)
ios-unit-tests 95.32% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...it_tests/DataStoreHubEventStreamHandlerTests.swift 97.87% <0.00%> (ø)
...plify_flutter/example/ios/Runner/AppDelegate.swift 100.00% <0.00%> (ø)
...mple/ios/unit_tests/QueryPaginationUnitTests.swift 100.00% <0.00%> (ø)
...plify_api_ios/example/ios/Runner/AppDelegate.swift 0.00% <0.00%> (ø)
...ter/example/ios/unit_tests/AtomicResultTests.swift 87.75% <0.00%> (ø)
.../example/ios/unit_tests/resources/SchemaData.swift 100.00% <0.00%> (ø)
...mple/ios/unit_tests/DataStorePluginUnitTests.swift 96.78% <0.00%> (ø)
...ple/ios/unit_tests/QuerySortBuilderUnitTests.swift 100.00% <0.00%> (ø)
...s/unit_tests/AmplifySerializedModelUnitTests.swift 100.00% <0.00%> (ø)
...e/example/ios/unit_tests/GetJsonFromFileUtil.swift 57.14% <0.00%> (ø)
... and 7 more

@dnys1 dnys1 marked this pull request as ready for review August 30, 2022 15:20
@dnys1 dnys1 force-pushed the feat/aft/changlog branch 2 times, most recently from cbf1f07 to 9a2630d Compare September 13, 2022 01:14
Copy link
Member

@Jordan-Nelson Jordan-Nelson left a comment

Choose a reason for hiding this comment

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

Some minor comments, but looks pretty good!

aft.yaml Outdated
ignore:
- synthetic_package

# Strongly connected components which should have major version bumps happen
Copy link
Member

Choose a reason for hiding this comment

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

All version bumps, no?

Suggested change
# Strongly connected components which should have major version bumps happen
# Strongly connected components which should have all version bumps happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've kind of changed the meaning of the word minor/major version throughout. Since true "major" version bumps, e.g. 0.x -> 1.x are so insanely rare and would not be handled by this script, I've repurposed breaking/major to be useful descriptions of, for example, making a breaking change to a low-level API or adding a new category, etc. where all the packages should be bumped in unison to the next version (0.5.x -> 0.6.0 or 1.0.x -> 1.1.0).

In this way, minor version bumps constitute everything else - for which they only apply to the affected packages. Changing a README in aws_common, for example, would be a "minor" version bump of just aws_common (e.g. 0.1.5 -> 0.1.6).

Copy link
Member

Choose a reason for hiding this comment

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

Okay. Does that mean amplify_auth_cognito could be on version 1.1.8 and amplify_datastore could be on version 1.1.7? I'm not clear if you are suggesting we change how we pin versions in amplify or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. I'm suggesting we don't pin versions anymore since there's no real need - component packages would always be at the same SemVer minor version though. The only thing that would change is the SemVer patch version.

@@ -14,3 +15,24 @@ A CLI tool for managing the Amplify Flutter repository.
- `get`: Runs `dart pub get`/`flutter pub get` for all packages
- `upgrade`: Runs `dart pub upgrade`/`flutter pub upgrade` for all packages
- `publish`: Runs `dart pub publish`/`flutter pub publish` for all packages which need publishing
- `version`: Bumps version using git history
Copy link
Member

Choose a reason for hiding this comment

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

Should this be version-bump, or something else a bit more specific? I could see this being confused with a command to get the current version of aft.

Somewhat related, I was thinking we should start bumping the version of aft when we have changes. That way we can check if the current activated version is the latest and print a warning if it is not (or maybe even throw for critical commands like version). It would be a nice protection against accidentally running commands with an outdated version of aft. I could tie some of that into this PR: #2022

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that sounds good. I've updated the commands from

aft version preview
aft version update

to

aft version-bump --preview
aft version-bump

I think #2022 makes sense for that change as well.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good. The readme still has version by the way.

Suggested change
- `version`: Bumps version using git history
- `version-bump`: Bumps version using git history

if (commits.isEmpty) {
// If there are no commits worth including, add a generic message about
// bug fixes/improvements.
nodes.add(Element.text('li', 'Minor bug fixes and improvements\n'));
Copy link
Member

Choose a reason for hiding this comment

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

There are two scenarios in which the list of commits could be empty, right?

  1. All the commits where non bug/feature/fix/etc. and where filtered out.
  2. The pre-filtered list of commits was already empty, which would mean this package is being bumped only because another package in its component group is being bumped.

This message seems appropriate for scenario 1, but not for scenario 2. Maybe for scenario 2 the message should be something like "internal dependency bumped to latest version"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In reality, this function was only ever being called in scenario 1. I've added an assert statement and updated the call sites to make it more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Okay. I see the assert in update(). Couldn't the value passed into update() be an empty set here? Will that be an issue if the assertion fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah whoops that's true. Still, scenario 2 never happens since a bump to component packages includes the commit. I think in the case you point out, we can either add the minor fixes or just not update the changelog. I'm cool with either approach.

.toList() ??
const [];
final description = commitMessage
.namedGroup('description')!
Copy link
Member

Choose a reason for hiding this comment

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

Silly edge case, but what happens for a message of "fix(auth)"? Is the description null or an empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would throw which I think is fine. We should have a CI action to check PR names probably to prevent something like that from happening.

await _updateChangelogs(preview: false);

logger.info('Changelogs successfully updated');
if (yes || prompt('Commit changes? (y/N) ').toLowerCase() == 'y') {
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this, but might be worth adding a yes/no prompt util.

  bool yesNoPrompt(String message) {
    final answer = prompt(message).toLowerCase();
    return answer == 'y' || answer == 'yes';
  }


/// Whether the package is an example package.
bool get isExample {
return pubspecInfo.pubspec.publishTo == 'none' ||
Copy link
Member

Choose a reason for hiding this comment

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

nit: isExample is a little misleading. Most of these are examples, but several are some sort of test package. I might suggest inverting this an making it isPublishable.

is || falsePositiveExamples.contains(name); needed? aft is the only package listed in falsePositiveExamples and it has publish_to: none in the pubspec.

Copy link
Contributor Author

@dnys1 dnys1 Sep 15, 2022

Choose a reason for hiding this comment

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

Yeah, this was a poorly named var, although it's intention is to know if it's an example app as opposed to a publishable or non-publishable development package. I've updated the definition to be more precise and to not require the false positives.

@dnys1 dnys1 force-pushed the feat/aft/changlog branch 4 times, most recently from 6593f10 to affef45 Compare September 21, 2022 15:52
@dnys1 dnys1 mentioned this pull request Nov 12, 2022
Dillon Nys added 16 commits November 16, 2022 12:25
There isn't any value with these being async since it's okay to block in this context. And sync makes everything easier to work with.

commit-id:6743d9d4
Fixes an issue in logger initialization where the initial plugin is not registered to the root plugin.

commit-id:063fbfe0
commit-id:de10a06e
commit-id:8261f867
Expand e2e tests to incude changelog/pubspec changes

commit-id:e9757240
commit-id:4509edf2
commit-id:14f44d17
Changelog updates should only be made with a non-empty list of commits.

commit-id:4eccafce
Refine the definition of `isExample` to be more precise and not require workarounds.

commit-id:51cc0ac0
The package libgit2dart, while it can be used in Dart-only packages tricks pub into thinking it has a dependency on Flutter. mono_repo cannot handle this discrepancy.

commit-id:ec27a8d2
Fixes constraints around publishing checks and which packages to consider for publishing.

commit-id:937ee042
Improves publish command checks by removing `pubspec_overrides` and not splitting the pre-publish and publish commands for a package.
@dnys1 dnys1 force-pushed the feat/aft/changlog branch 2 times, most recently from 40c1ad0 to 5b783cf Compare December 18, 2022 18:40
Adds workaround for dart-lang/pub#3563 which requires using Flutter's `dart` command if a package lists a Flutter SDK constraint even if the package does not depend on Flutter.
Instead of trying to install it (since the latest version is not available in apt)
checked_yaml: ^2.0.0
cli_util: ^0.3.5
collection: ^1.16.0
flutter_tools:
git:
url: https://github.com/flutter/flutter.git
ref: f186e1bd3ed0c8471ad9b52929f70292621ab796
ref: 117a83a4a7178c328d296748bd93ff388724cb67
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to change this to 7a743c8816fd8ff7df99858c545c1dbe396d1103 in my env to run due to error, dk if anyone else has this issue.

Copy link
Contributor Author

@dnys1 dnys1 Jan 10, 2023

Choose a reason for hiding this comment

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

What was the error? I've updated it but I haven't noticed any issues locally or in CI.

These dependency versions will need to be updated consistently, unfortunately, since both repos pin dependencies. It's probably worth some more investigation how to improve the efficiency of the pub get commands without including both these deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the error off hand and it was a few weeks ago but it was a version incompatibility thing and the ref I mentioned updated the version to be the one that matches other requirement. flutter/flutter@7a743c8#diff-a3d4dfa7e9543c501bc1aee57d5502b657d6f5469c09fb42b8c9d516ec53df66, maybe frontend_server_client.

Copy link
Member

@HuiSF HuiSF left a comment

Choose a reason for hiding this comment

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

Could you double check on Travis's comment? Otherwise LGTM

aft.yaml Outdated
Comment on lines 42 to 44
- amplify_storage_s3
- amplify_storage_s3_android
- amplify_storage_s3_ios
Copy link
Member

Choose a reason for hiding this comment

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

Small reminder can update/remove these packages now 😊

@@ -17,3 +18,24 @@ A CLI tool for managing the Amplify Flutter repository.
- `get`: Runs `dart pub get`/`flutter pub get` for all packages
- `upgrade`: Runs `dart pub upgrade`/`flutter pub upgrade` for all packages
- `publish`: Runs `dart pub publish`/`flutter pub publish` for all packages which need publishing
- `version-bump`: Bumps version using git history
Copy link
Member

Choose a reason for hiding this comment

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

As changelog and version-bump are separate commands, the release PR will always need to contain 2 commits - one for each?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the changelog command for now - it's all one command

Dillon Nys added 5 commits January 9, 2023 16:49
When `dev_dependencies` contains versions of the published packages different than those on pub.dev (for example when using path deps or `any`), this causes issues during pre-publish verification.

By simply keeping these constraints up-to-date as well, we lose nothing (since they are overridden in local development via linking), but gain stronger confidence in the pre-publish step.
@dnys1 dnys1 merged commit 29c785d into aws-amplify:next Jan 10, 2023
@dnys1 dnys1 deleted the feat/aft/changlog branch January 10, 2023 18:03
dnys1 pushed a commit to dnys1/amplify-flutter that referenced this pull request Jan 10, 2023
dnys1 pushed a commit to dnys1/amplify-flutter that referenced this pull request Jan 10, 2023
dnys1 added a commit that referenced this pull request Jan 10, 2023
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