Skip to content

Conversation

christopherfujino
Copy link
Contributor

@christopherfujino christopherfujino commented May 7, 2020

Description

This PR does the following:

  1. Adds a --force flag, since dev releases can have cherry picks.
  2. Changes the logic that detects the previous version to check the tip of the dev branch, rather than picking up the nearest ancestor, because a published cherry pick won't be an ancestor of the next dev release.
  3. Refactors the code to allow more extensive unit testing.

Related Issues

FIxes #56340

Tests

Added extensive unit tests, exercising existing changed logic and the pre-existing logic.

@christopherfujino christopherfujino marked this pull request as ready for review May 7, 2020 01:35
runGit('push $origin HEAD:dev', 'land the new version on the "dev" branch');
git.run('push $origin $version', 'publish the version');
git.run(
'push ${force ? "--force " : ""}$origin HEAD:dev',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is change 1

// describe the latest dev release
const String ref = 'refs/heads/dev';
return git.getOutput(
'describe --match $glob --exact-match --tags $ref',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is change 2

'obtain last released version number',
);
}

Match parseFullTag(String version) {
// of the form: x.y.z-m.n.pre-c-g<revision>
final RegExp versionPattern = RegExp(
r'^(\d+)\.(\d+)\.(\d+)-(\d+)\.(\d+)\.pre-(\d+)-g([a-f0-9]+)$');
r'^(\d+)\.(\d+)\.(\d+)-(\d+)\.(\d+)\.pre$');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of change 2. Instead of describing HEAD, we grab the exact tag at the tip of refs/heads/dev.

_reportGitFailureAndExit(result, explanation);
return null; // for the analyzer's sake
}
class Git {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created this wrapper so we can inject mocks for tests.

'obtain last released version number',
);
}

Match parseFullTag(String version) {
// of the form: x.y.z-m.n.pre-c-g<revision>
final RegExp versionPattern = RegExp(
r'^(\d+)\.(\d+)\.(\d+)-(\d+)\.(\d+)\.pre-(\d+)-g([a-f0-9]+)$');
r'^(\d+)\.(\d+)\.(\d+)-(\d+)\.(\d+)\.pre$');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the comment need to updated too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

}
class Git {
const Git();
String getOutput(String command, String explanation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
String getOutput(String command, String explanation) {
String getOutput(String command, String explanation) {

}
const String pattern = r'The current directory is not a Flutter '
'repository checkout with a correctly configured upstream remote.';
expect(exception.toString(), contains(pattern));
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails to catch something, this will throw an NPE here instead of failing to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently null implements .toString()

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, its null!

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 guess it's still safer to use exception?.toString() though, so it doesn't match 'null'...

const String pattern = r'Your git repository is not clean. Try running '
'"git clean -fd". Warning, this will delete files! Run with -n to find '
'out which ones.';
expect(exception.toString(), contains(pattern));
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM with nits. I would make sure the comments describing the tag format are all up to date too

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[flutter_tools] roll_dev.dart does not work after cherry-picked release
4 participants