-
Notifications
You must be signed in to change notification settings - Fork 164
Send changelog excerpt in package uploaded email. #8913
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
Changes from all commits
ae3ec55
69465c1
9586f1b
1736dcf
0648f97
8416d5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import 'package:_pub_shared/data/account_api.dart' as account_api; | |
import 'package:_pub_shared/data/package_api.dart' as api; | ||
import 'package:_pub_shared/utils/sdk_version_cache.dart'; | ||
import 'package:clock/clock.dart'; | ||
import 'package:collection/collection.dart'; | ||
import 'package:convert/convert.dart'; | ||
import 'package:crypto/crypto.dart'; | ||
import 'package:gcloud/service_scope.dart' as ss; | ||
|
@@ -21,6 +22,8 @@ import 'package:pub_dev/package/tarball_storage.dart'; | |
import 'package:pub_dev/scorecard/backend.dart'; | ||
import 'package:pub_dev/service/async_queue/async_queue.dart'; | ||
import 'package:pub_dev/service/rate_limit/rate_limit.dart'; | ||
import 'package:pub_dev/shared/changelog.dart'; | ||
import 'package:pub_dev/shared/monitoring.dart'; | ||
import 'package:pub_dev/shared/versions.dart'; | ||
import 'package:pub_dev/task/backend.dart'; | ||
import 'package:pub_package_reader/pub_package_reader.dart'; | ||
|
@@ -1168,6 +1171,15 @@ class PackageBackend { | |
.run() | ||
.toList(); | ||
|
||
final changelogExcerpt = _createChangelogExcerpt( | ||
versionKey: newVersion.qualifiedVersionKey, | ||
changelogContent: entities.changelogAsset?.textContent, | ||
); | ||
if (changelogExcerpt != null && changelogExcerpt.isNotEmpty) { | ||
uploadMessages | ||
.add('Excerpt of the changelog:\n```\n$changelogExcerpt\n```'); | ||
} | ||
|
||
// Add the new package to the repository by storing the tarball and | ||
// inserting metadata to datastore (which happens atomically). | ||
final (pv, outgoingEmail) = await withRetryTransaction(db, (tx) async { | ||
|
@@ -1315,6 +1327,50 @@ class PackageBackend { | |
return (pv, uploadMessages); | ||
} | ||
|
||
String? _createChangelogExcerpt({ | ||
required QualifiedVersionKey versionKey, | ||
required String? changelogContent, | ||
}) { | ||
if (changelogContent == null) { | ||
return null; | ||
} | ||
try { | ||
final parsed = ChangelogParser().parseMarkdownText(changelogContent); | ||
final version = parsed.releases | ||
.firstWhereOrNull((r) => r.version == versionKey.version); | ||
if (version == null) { | ||
return null; | ||
} | ||
final text = version.content.asMarkdownText; | ||
|
||
/// Limit the changelog to 10 lines, 75 characters each: | ||
final lines = text.split('\n'); | ||
final excerpt = lines | ||
// prevent accidental HTML-tag creation | ||
.map((line) => line | ||
.replaceAll('<', '[') | ||
.replaceAll('>', ']') | ||
.replaceAll('&', ' ') | ||
.trim()) | ||
// filter empty or decorative lines to maximalize usefulness | ||
.where((line) => | ||
line.isNotEmpty && | ||
line != '-' && // empty list item | ||
line != '1.' && // empty list item | ||
!line.startsWith('```') && // also removes the need to escape it | ||
!line.startsWith('---')) | ||
.take(10) | ||
.map((line) => | ||
line.length < 76 ? line : '${line.substring(0, 70)}[...]') | ||
.join('\n'); | ||
return excerpt; | ||
} catch (e, st) { | ||
_logger.pubNoticeShout('changelog-parse-error', | ||
'Unable to parse changelog for $versionKey', e, st); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this log the package name also? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it is |
||
return null; | ||
} | ||
} | ||
|
||
/// The post-upload tasks are not critical and could fail without any impact on | ||
/// the uploaded package version. Important operations (e.g. email sending) are | ||
/// retried periodically, others (e.g. triggering re-analysis of dependent | ||
|
@@ -1903,6 +1959,9 @@ class _UploadEntities { | |
this.packageVersionInfo, | ||
this.assets, | ||
); | ||
|
||
late final changelogAsset = | ||
assets.firstWhereOrNull((e) => e.kind == AssetKind.changelog); | ||
} | ||
|
||
class DerivedPackageVersionEntities { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,6 +128,8 @@ void main() { | |
expect(email.subject, 'Package uploaded: new_package 1.2.3'); | ||
expect(email.bodyText, | ||
contains('https://pub.dev/packages/new_package/versions/1.2.3\n')); | ||
// No relevant changelog entry for this version. | ||
expect(email.bodyText, isNot(contains('Excerpt of the changelog'))); | ||
|
||
final audits = await auditBackend.listRecordsForPackageVersion( | ||
'new_package', '1.2.3'); | ||
|
@@ -1193,7 +1195,9 @@ void main() { | |
final p1 = await packageBackend.lookupPackage('oxygen'); | ||
expect(p1!.versionCount, 3); | ||
final tarball = await packageArchiveBytes( | ||
pubspecContent: generatePubspecYaml('oxygen', '3.0.0')); | ||
pubspecContent: generatePubspecYaml('oxygen', '3.0.0'), | ||
changelogContent: | ||
'# Changelog\n\n## v3.0.0\n\nSome bug fixes:\n- one,\n- two\n\n'); | ||
final message = await createPubApiClient(authToken: adminClientToken) | ||
.uploadPackageBytes(tarball); | ||
expect(message.success.message, contains('Successfully uploaded')); | ||
|
@@ -1210,6 +1214,15 @@ void main() { | |
expect(email.subject, 'Package uploaded: oxygen 3.0.0'); | ||
expect(email.bodyText, | ||
contains('https://pub.dev/packages/oxygen/versions/3.0.0\n')); | ||
expect( | ||
email.bodyText, | ||
contains('\n' | ||
'Excerpt of the changelog:\n' | ||
'```\n' | ||
'Some bug fixes:\n' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we consider also including the header of the relevant section? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure. For most packages, it will be only the version string. Maybe if the changelog itself is not too long? |
||
'- one,\n' | ||
'- two\n' | ||
'```\n\n')); | ||
|
||
await nameTracker.reloadFromDatastore(); | ||
final lastPublished = | ||
|
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 we return null if the excerpt is empty/only whitespace?
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.
We are filtering out most of the empty spaces and lines, maybe empty list items will be still kept. I'll add a bit more conditions to prevent / post-filter those.
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 was thinking if the "whole message" is whitespace only - it would look weird in the email.