[tool] version-check publish-check commands can check against pub #3840
Conversation
cyanglaz
changed the title
| help: | ||
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | ||
| 'The possible values are:\n' | ||
| ' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks. \n' |
There was a problem hiding this comment.
s/need to/that needs to be/
| help: | ||
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | ||
| 'The possible values are:\n' | ||
| ' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks. \n' |
There was a problem hiding this comment.
Remove training space since there's a \n
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | ||
| 'The possible values are:\n' | ||
| ' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks. \n' | ||
| ' $_resultNoPublish: There are no packages need to be published. Either no pubspec change detected or the version has already been published. \n' |
There was a problem hiding this comment.
s/the version/all versions/, since there can be multiple packages.
| @@ -26,9 +29,22 @@ class PublishCheckCommand extends PluginCommand { | |||
| 'the SDK constraint is a pre-release version, is ignored.', | |||
| defaultsTo: false, | |||
| ); | |||
| argParser.addFlag(_logStatusFlag, | |||
| help: | |||
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | |||
There was a problem hiding this comment.
Mixing output for humans and output for machines is generally not a good idea; I would rather we make a --machine flag that switches entirely to an output meant for parsing.
| final PubVersionFinder pubVersionFinder = PubVersionFinder( | ||
| package: packageName, httpClient: httpClient ?? http.Client()); | ||
| final PubVersionFinderResponse pubVersionFinderResponse = | ||
| await pubVersionFinder.getPackageVersion(); |
There was a problem hiding this comment.
Don't we want a try/catch here to deal with issues reaching pub?
There was a problem hiding this comment.
We don't need try-catch, but we should check the status of pubVersionFinderResponse. Done.
| help: | ||
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | ||
| 'The possible values are:\n' | ||
| ' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks. \n' |
| help: | ||
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | ||
| 'The possible values are:\n' | ||
| ' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks. \n' |
| 'Logs the check-publish final status to a defined string as the last line of the command output.\n' | ||
| 'The possible values are:\n' | ||
| ' $_resultNeedsPublish: There is at least one package need to be published. They also passed all publish checks. \n' | ||
| ' $_resultNoPublish: There are no packages need to be published. Either no pubspec change detected or the version has already been published. \n' |
| final PubVersionFinder pubVersionFinder = PubVersionFinder( | ||
| package: packageName, httpClient: httpClient ?? http.Client()); | ||
| final PubVersionFinderResponse pubVersionFinderResponse = | ||
| await pubVersionFinder.getPackageVersion(); |
There was a problem hiding this comment.
We don't need try-catch, but we should check the status of pubVersionFinderResponse. Done.
script/tool/CHANGELOG.md
Outdated
| ## NEXT | ||
|
|
||
| - Add `against-pub` flag for version-check. | ||
| - Add `log-status` flag for publish-check. |
There was a problem hiding this comment.
This needs to be updated.
script/tool/lib/src/common.dart
Outdated
| /// The pub host url, defaults to `https://pub.dev`. | ||
| final String pubHost; | ||
|
|
||
| /// The http client, can override for testing. |
There was a problem hiding this comment.
If it's required, then it's not really an override for testing.
script/tool/lib/src/common.dart
Outdated
| final http.Response httpResponse; | ||
| } | ||
|
|
||
| /// An enum represents the result of [PubVersionFinder]. |
script/tool/lib/src/common.dart
Outdated
| @@ -549,6 +550,97 @@ class ProcessRunner { | |||
| } | |||
| } | |||
|
|
|||
| /// Finding version of [package] that is published on pub. | |||
| /// | |||
| /// Note: you should manually close the [httpClient] when done using the finder. | |||
There was a problem hiding this comment.
This seems worth noting on the constructor rather than the class.
script/tool/lib/src/common.dart
Outdated
| final http.Client httpClient; | ||
|
|
||
| /// Get the package version on pub. | ||
| Future<PubVersionFinderResponse> getPackageVersion() async { |
There was a problem hiding this comment.
It seems like the class would make more sense as a class if package were an argument here rather than on the constructor. I.e., you configure it for a given server/network setup, then you can query packages.
script/tool/lib/src/common.dart
Outdated
|
|
||
| /// The version finder failed to locate the package. | ||
| /// | ||
| /// This is usually OK when the package is new. |
There was a problem hiding this comment.
Why "usually"? When would it not be okay for a new package not to be found?
There was a problem hiding this comment.
reworded, this was not what I meant.
| final Colorize passedMessage = | ||
| Colorize('All packages passed publish check!')..green(); | ||
| print(passedMessage); | ||
| // This has to be last output of this command because it is promised in the help section. |
| published = pubVersionFinderResponse.versions.contains(version); | ||
| break; | ||
| case PubVersionFinderResult.fail: | ||
| printErrorAndExit(errorMessage: ''' |
There was a problem hiding this comment.
Won't this break the --machine case, because the final message will never print?
There was a problem hiding this comment.
I was thinking this should return something, rather than bailing completely. Is the reason for this that we assume that all the other checks will fail the same way because it's likely a network issue?
If we do want to keep this construction, this line (and the line above) need comments explaining why they are doing what they are doing, since it's non-obvious.
There was a problem hiding this comment.
ah ok, I updated to print the message and handled the error later
script/tool/CHANGELOG.md
Outdated
| ## NEXT | ||
|
|
||
| - Add `against-pub` flag for version-check. | ||
| - Add `log-status` flag for publish-check. |
script/tool/lib/src/common.dart
Outdated
| @@ -549,6 +550,97 @@ class ProcessRunner { | |||
| } | |||
| } | |||
|
|
|||
| /// Finding version of [package] that is published on pub. | |||
| /// | |||
| /// Note: you should manually close the [httpClient] when done using the finder. | |||
script/tool/lib/src/common.dart
Outdated
| /// The pub host url, defaults to `https://pub.dev`. | ||
| final String pubHost; | ||
|
|
||
| /// The http client, can override for testing. |
script/tool/lib/src/common.dart
Outdated
| final http.Client httpClient; | ||
|
|
||
| /// Get the package version on pub. | ||
| Future<PubVersionFinderResponse> getPackageVersion() async { |
script/tool/lib/src/common.dart
Outdated
| final http.Response httpResponse; | ||
| } | ||
|
|
||
| /// An enum represents the result of [PubVersionFinder]. |
script/tool/lib/src/common.dart
Outdated
|
|
||
| /// The version finder failed to locate the package. | ||
| /// | ||
| /// This is usually OK when the package is new. |
There was a problem hiding this comment.
reworded, this was not what I meant.
| final Colorize passedMessage = | ||
| Colorize('All packages passed publish check!')..green(); | ||
| print(passedMessage); | ||
| // This has to be last output of this command because it is promised in the help section. |
| published = pubVersionFinderResponse.versions.contains(version); | ||
| break; | ||
| case PubVersionFinderResult.fail: | ||
| printErrorAndExit(errorMessage: ''' |
| published = pubVersionFinderResponse.versions.contains(version); | ||
| break; | ||
| case PubVersionFinderResult.fail: | ||
| printErrorAndExit(errorMessage: ''' |
There was a problem hiding this comment.
I was thinking this should return something, rather than bailing completely. Is the reason for this that we assume that all the other checks will fail the same way because it's likely a network issue?
If we do want to keep this construction, this line (and the line above) need comments explaining why they are doing what they are doing, since it's non-obvious.
| Error fetching version on pub for $packageName. | ||
| HTTP Status ${pubVersionFinderResponse.httpResponse.statusCode} | ||
| HTTP response: ${pubVersionFinderResponse.httpResponse.body} | ||
| '''); |
There was a problem hiding this comment.
I'm realizing that failures here are going to be almost impossible to debug, because we won't have any information in the log.
Perhaps we should consider doing something like a JSON dictionary of output, with status and detailed log messages as separate keys? That would mean we'd need to consume the result with something like a Dart script that can parse the JSON, rather than just a one-liner in the GitHub Action. Thoughts?
There was a problem hiding this comment.
Good Idea, updated the code to do exactly that
| throw ToolExit(1); | ||
| } else { | ||
| final Colorize passedMessage = | ||
| Colorize('All packages passed publish check!')..green(); |
There was a problem hiding this comment.
Looking at the new tests, it's very strange to have terminal color commands in the --machine output. I think we should replace all the Colorize calls with a utility method like:
_printImportantStatusMessage(String message, {@required bool isError}) {
if (argResults[_machineFlag] as bool) {
print('${isError ? 'ERROR' : 'SUCCESS'}: $message');
} else {
final Colorize colorizedMessage = Colorize(message);
if (isError) {
colorizedMessage.red();
} else {
colorizedMessage.green();
}
print(colorizedMessage);
}
}
There was a problem hiding this comment.
I updated so that both machine mesasge and human message include the error and success prefix.
| throw ToolExit(1); | ||
| } else { | ||
| final Colorize passedMessage = | ||
| Colorize('All packages passed publish check!')..green(); |
There was a problem hiding this comment.
I updated so that both machine mesasge and human message include the error and success prefix.
…utter#3840) Add a PubVersionFinder class to easily fetch the version from pub. Add an against-pub flag to check-version command, which allows it to check the version against pub server Make the 'publish-check' command to check against pub to determine if the specific versions of packages need to be published. Add a log-status flag, which allows the publish-check command to log the final status of the result. This helps other ci tools to easily grab the results and use it to determine what to do next. See option 3 in flutter/flutter#81444 This PR also fixes some tests. partially flutter/flutter#81444
Add a
PubVersionFinderclass to easily fetch the version from pub.Add an
against-pubflag tocheck-versioncommand, which allows it to check the version against pub serverMake the 'publish-check' command to check against pub to determine if the specific versions of packages need to be published.
Add a
log-statusflag, which allows thepublish-checkcommand to log the final status of the result. This helps other ci tools to easily grab the results and use it to determine what to do next. See option 3 in flutter/flutter#81444This PR also fixes some tests.
partially flutter/flutter#81444
Pre-launch Checklist
dart format. See plugin_tool format)[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.