-
Notifications
You must be signed in to change notification settings - Fork 249
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
2854886
to
048f836
Compare
a89dfc0
to
8ce9d83
Compare
cbf1f07
to
9a2630d
Compare
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.
Some minor comments, but looks pretty good!
aft.yaml
Outdated
ignore: | ||
- synthetic_package | ||
|
||
# Strongly connected components which should have major version bumps 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.
All version bumps, no?
# Strongly connected components which should have major version bumps happen | |
# Strongly connected components which should have all version bumps 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'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
).
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.
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.
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.
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.
packages/aft/README.md
Outdated
@@ -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 |
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.
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
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 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.
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.
Sounds good. The readme still has version
by the way.
- `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')); |
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.
There are two scenarios in which the list of commits could be empty, right?
- All the commits where non bug/feature/fix/etc. and where filtered out.
- 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"?
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.
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.
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.
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?
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.
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')! |
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.
Silly edge case, but what happens for a message of "fix(auth)"
? Is the description null or an empty string?
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.
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') { |
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.
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';
}
packages/aft/lib/src/models.dart
Outdated
|
||
/// Whether the package is an example package. | ||
bool get isExample { | ||
return pubspecInfo.pubspec.publishTo == 'none' || |
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.
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.
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, 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.
6593f10
to
affef45
Compare
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
commit-id:0c17379d
Fixes an issue in logger initialization where the initial plugin is not registered to the root plugin. commit-id:063fbfe0
commit-id:c45852ad
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
commit-id:8544885f
Refine the definition of `isExample` to be more precise and not require workarounds. commit-id:51cc0ac0
commit-id:0526b477
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.
Use build tag (`+`) for patch releases.
40c1ad0
to
5b783cf
Compare
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.
5b783cf
to
78c6448
Compare
Remove unncessary commas
78c6448
to
1415fb7
Compare
Instead of trying to install it (since the latest version is not available in apt)
85cc127
to
8697e06
Compare
8697e06
to
bed7229
Compare
packages/aft/pubspec.yaml
Outdated
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 |
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 had to change this to 7a743c8816fd8ff7df99858c545c1dbe396d1103
in my env to run due to error, dk if anyone else has this issue.
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.
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.
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 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.
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.
Could you double check on Travis's comment? Otherwise LGTM
aft.yaml
Outdated
- amplify_storage_s3 | ||
- amplify_storage_s3_android | ||
- amplify_storage_s3_ios |
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.
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 |
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.
As changelog
and version-bump
are separate commands, the release PR will always need to contain 2 commits - one for each?
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've removed the changelog command for now - it's all one command
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.
7b70fb4
to
65b1f71
Compare
Broke due to changes in aws-amplify#2068
Broke due to changes in aws-amplify#2068
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 ortee
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.packages
be the set of all packages in the repo which are publishable topub.dev
.P
inpackages
:component
be the component ofP
, if any.baseRef
be the commit of the last release ofP
.headRef
be the releaseable commit ofP
(defaults toHEAD
).history
be the list of git commits in the rangebaseRef..headRef
which affectedP
, i.e. those commits which included changes to files inP
.nextVersion = currentVersion
.commit
inhistory
:commit
is a version bump (i.e.chore(version)
), ignore it.commit
is a merge commit, update dependencies based on the packages changed by the commit.commit
is a breaking change (i.e.feat(auth)!
), setbumpType = breaking
.commit
's type isfeat
, setbumpType = nonBreaking
.bumpType = patch
.commit
is a noteworthy change (scope is one offeat
,fix
,bug
,perf
, orrevert
or it's a breaking change), setincludeInChangelog = true
.proposedVersion = currentVersion.bump(bumpType)
nextVersion = max(nextVersion, proposedVersion)
nextVersion > currentVersion
:pubspec.yaml
, setversion = nextVersion
includeInChangelog
:CHANGELOG.md
with an entry forcommit
.bumpType == breaking
:Q
which directly depends onP
:Q
withbumpType = patch
andincludeInChangelog = false
.Q
's constraint onP
.bumpType == breaking
orbumpType == nonBreaking
andcomponent != null
:Q
incomponent
:Q
with the samebumpType
asP
andincludeInChangelog = false
.component
has a summary package:CHANGELOG.md
in the summary package withcommit
.Q
which was affected bycommit
:Q
's constraint onP
usingnextVersion
.