diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ce115e32fa..dd2e56de4b 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -320,9 +320,13 @@ jobs: prerelease_flag=(--prerelease) fi - cargo xtask changelog render \ + if ! cargo xtask changelog render \ --release-tag "${TAG}" \ - > "${RUNNER_TEMP}/release-notes.md" + > "${RUNNER_TEMP}/release-notes.md"; then + echo "::warning title=Release notes generation failed::Using fallback release notes." + printf 'Release %s\n\nRelease notes could not be generated automatically.\n' "${TAG}" \ + > "${RUNNER_TEMP}/release-notes.md" + fi gh release create "${TAG}" \ --repo "${REPO}" \ diff --git a/xtask/src/changelog.rs b/xtask/src/changelog.rs index 522047cab2..85d3ddf5d0 100644 --- a/xtask/src/changelog.rs +++ b/xtask/src/changelog.rs @@ -67,7 +67,13 @@ struct ReleaseNoteEntry { #[derive(Debug)] struct InvalidChangelogEntry { - pr_number: u64, + source: InvalidChangelogSource, reason: String, order: usize, } + +#[derive(Debug)] +enum InvalidChangelogSource { + PullRequest(u64), + Commit(String), +} diff --git a/xtask/src/changelog/release.rs b/xtask/src/changelog/release.rs index 9b058ee2f0..1e39209691 100644 --- a/xtask/src/changelog/release.rs +++ b/xtask/src/changelog/release.rs @@ -5,7 +5,7 @@ use anyhow::{Context, Result, bail, ensure}; use semver::Version; use super::pr::{self, ChangelogDocument}; -use super::{InvalidChangelogEntry, ReleaseNoteEntry}; +use super::{InvalidChangelogEntry, InvalidChangelogSource, ReleaseNoteEntry}; pub(super) struct ChangelogEntries { pub(super) entries: Vec, @@ -39,9 +39,11 @@ pub(super) fn release_changelog_entries(release_tag: &str) -> Result Result { @@ -63,9 +65,9 @@ pub(super) fn current_changelog_entries() -> Result { } let repo = github_repo()?; - let pull_requests = - pull_requests_for_commits(&repo, &commits, MissingPullRequest::WarnAndSkip)?; - let changelog = changelog_entries_for_pull_requests(&repo, &pull_requests)?; + let pull_requests = pull_requests_for_commits(&repo, &commits)?; + let mut changelog = changelog_entries_for_pull_requests(&repo, &pull_requests.pull_requests); + changelog.invalid_entries.extend(pull_requests.invalid_entries); Ok(CurrentChangelog { title: format!("Changes since {previous_stable_tag}"), @@ -79,10 +81,14 @@ struct ReleaseTag { version: Version, } -#[derive(Clone, Copy)] -enum MissingPullRequest { - Error, - WarnAndSkip, +struct PullRequests { + pull_requests: Vec, + invalid_entries: Vec, +} + +struct AssociatedPullRequest { + number: u64, + order: usize, } enum CommitPullRequests { @@ -189,50 +195,54 @@ fn github_repo() -> Result { Ok(repo.to_owned()) } -fn pull_requests_for_commits( +fn pull_requests_for_commits(repo: &str, commits: &[String]) -> Result { + pull_requests_for_commits_with(repo, commits, pull_requests_for_commit) +} + +fn pull_requests_for_commits_with( repo: &str, commits: &[String], - missing_pull_request: MissingPullRequest, -) -> Result> { + mut pull_requests_for_commit: F, +) -> Result +where + F: FnMut(&str, &str) -> Result, +{ let mut pull_requests = Vec::new(); + let mut invalid_entries = Vec::new(); - for commit in commits { + for (order, commit) in commits.iter().enumerate() { let commit_pull_requests = pull_requests_for_commit(repo, commit) .with_context(|| format!("fetching pull requests associated with commit {commit}"))?; + let CommitPullRequests::Found(commit_pull_requests) = commit_pull_requests else { - match missing_pull_request { - MissingPullRequest::Error => { - bail!("commit {commit} was not found in GitHub repository {repo}"); - }, - MissingPullRequest::WarnAndSkip => { - eprintln!( - "warning: skipping commit {commit}; not found in GitHub repository {repo}" - ); - continue; - }, - } + invalid_entries.push(InvalidChangelogEntry { + source: InvalidChangelogSource::Commit(commit.clone()), + reason: format!("commit was not found in GitHub repository {repo}"), + order, + }); + continue; }; if commit_pull_requests.is_empty() { - match missing_pull_request { - MissingPullRequest::Error => { - bail!("commit {commit} has no associated pull request"); - }, - MissingPullRequest::WarnAndSkip => { - eprintln!("warning: skipping commit {commit}; no associated pull request"); - continue; - }, - } + invalid_entries.push(InvalidChangelogEntry { + source: InvalidChangelogSource::Commit(commit.clone()), + reason: "no associated pull request".to_owned(), + order, + }); + continue; } for pull_request in commit_pull_requests { - if !pull_requests.contains(&pull_request) { - pull_requests.push(pull_request); + if !pull_requests + .iter() + .any(|entry: &AssociatedPullRequest| entry.number == pull_request) + { + pull_requests.push(AssociatedPullRequest { number: pull_request, order }); } } } - Ok(pull_requests) + Ok(PullRequests { pull_requests, invalid_entries }) } fn pull_requests_for_commit(repo: &str, commit: &str) -> Result { @@ -267,22 +277,46 @@ fn pull_requests_for_commit(repo: &str, commit: &str) -> Result Result { + pull_requests: &[AssociatedPullRequest], +) -> ChangelogEntries { + changelog_entries_for_pull_requests_with(repo, pull_requests, pull_request_body) +} + +fn changelog_entries_for_pull_requests_with( + repo: &str, + pull_requests: &[AssociatedPullRequest], + mut pull_request_body: F, +) -> ChangelogEntries +where + F: FnMut(&str, u64) -> Result, +{ let mut entries = Vec::new(); let mut invalid_entries = Vec::new(); - for (order, pull_request) in pull_requests.iter().enumerate() { - let body = pull_request_body(repo, *pull_request) - .with_context(|| format!("fetching pull request #{pull_request} body"))?; + for pull_request in pull_requests { + let body = match pull_request_body(repo, pull_request.number) { + Ok(body) => body, + Err(err) => { + eprintln!( + "warning: could not fetch pull request #{} body: {err:#}", + pull_request.number + ); + invalid_entries.push(InvalidChangelogEntry { + source: InvalidChangelogSource::PullRequest(pull_request.number), + reason: "pull request body could not be fetched".to_owned(), + order: pull_request.order, + }); + continue; + }, + }; let document = match pr::changelog_document_from_pr_body(&body) { Ok(document) => document, Err(err) => { invalid_entries.push(InvalidChangelogEntry { - pr_number: *pull_request, + source: InvalidChangelogSource::PullRequest(pull_request.number), reason: normalize_description(&format!("{err:#}")), - order, + order: pull_request.order, }); continue; }, @@ -294,16 +328,16 @@ fn changelog_entries_for_pull_requests( for entry in pr_entries { entries.push(ReleaseNoteEntry { - pr_number: *pull_request, + pr_number: pull_request.number, scope: entry.scope, impact: entry.impact, description: normalize_description(&entry.description), - order, + order: pull_request.order, }); } } - Ok(ChangelogEntries { entries, invalid_entries }) + ChangelogEntries { entries, invalid_entries } } fn pull_request_body(repo: &str, pull_request: u64) -> Result { @@ -366,7 +400,142 @@ fn normalize_description(description: &str) -> String { #[cfg(test)] mod tests { - use super::{ReleaseTag, previous_release_tag_from}; + use anyhow::anyhow; + + use super::{ + AssociatedPullRequest, + CommitPullRequests, + ReleaseTag, + changelog_entries_for_pull_requests_with, + previous_release_tag_from, + pull_requests_for_commits_with, + }; + use crate::changelog::render; + + #[test] + fn missing_pull_requests_are_rendered_as_changelog_issues() { + let commits = vec![ + "0123456789abcdef0123456789abcdef01234567".to_owned(), + "abcdef0123456789abcdef0123456789abcdef01".to_owned(), + ]; + let changelog = + pull_requests_for_commits_with("0xMiden/node", &commits, |_repo, commit| { + Ok(if commit.starts_with('0') { + CommitPullRequests::Found(Vec::new()) + } else { + CommitPullRequests::CommitNotFound + }) + }) + .unwrap(); + + let notes = render::release_notes("Release v1.2.3", &[], &changelog.invalid_entries); + + assert_eq!( + notes, + r"Release v1.2.3 + +## Changelog Entries Requiring Attention + +- Missing PR for commit 0123456789ab: no associated pull request +- Missing PR for commit abcdef012345: commit was not found in GitHub repository 0xMiden/node + +No release-note-worthy changes. +" + ); + } + + #[test] + fn associated_pull_requests_keep_first_commit_order_and_are_deduplicated() { + let commits = vec!["first".to_owned(), "missing".to_owned(), "last".to_owned()]; + let pull_requests = + pull_requests_for_commits_with("0xMiden/node", &commits, |_repo, commit| { + Ok(CommitPullRequests::Found(match commit { + "first" => vec![42], + "missing" => Vec::new(), + "last" => vec![42, 43], + _ => unreachable!(), + })) + }) + .unwrap(); + + let associations = pull_requests + .pull_requests + .iter() + .map(|pull_request| (pull_request.number, pull_request.order)) + .collect::>(); + + assert_eq!(associations, vec![(42, 0), (43, 2)]); + assert_eq!(pull_requests.invalid_entries.len(), 1); + assert_eq!(pull_requests.invalid_entries[0].order, 1); + } + + #[test] + fn pull_request_lookup_failure_aborts_collection() { + let commit = "0123456789abcdef0123456789abcdef01234567".to_owned(); + let result = pull_requests_for_commits_with( + "0xMiden/node", + std::slice::from_ref(&commit), + |_repo, _commit| Err(anyhow!("authentication failed")), + ); + + let Err(err) = result else { + panic!("expected pull request lookup to fail"); + }; + assert!( + err.to_string() + .contains("fetching pull requests associated with commit 0123456789abcdef") + ); + } + + #[test] + fn unavailable_pull_request_is_rendered_as_a_changelog_issue() { + let pull_requests = [AssociatedPullRequest { number: 42, order: 0 }]; + let changelog = changelog_entries_for_pull_requests_with( + "0xMiden/node", + &pull_requests, + |_repo, _pull_request| Err(anyhow!("pull request not found")), + ); + + let notes = + render::release_notes("Release v1.2.3", &changelog.entries, &changelog.invalid_entries); + + assert_eq!( + notes, + r"Release v1.2.3 + +## Changelog Entries Requiring Attention + +- Broken PR #42: pull request body could not be fetched + +No release-note-worthy changes. +" + ); + } + + #[test] + fn malformed_pull_request_is_rendered_as_a_changelog_issue() { + let pull_requests = [ + AssociatedPullRequest { number: 42, order: 0 }, + AssociatedPullRequest { number: 43, order: 1 }, + ]; + let changelog = changelog_entries_for_pull_requests_with( + "0xMiden/node", + &pull_requests, + |_repo, pull_request| { + Ok(match pull_request { + 42 => "## Summary\n\nNo changelog here.\n".to_owned(), + 43 => "## Changelog\n\n```toml\n[[entry]\n```\n".to_owned(), + _ => unreachable!(), + }) + }, + ); + + let notes = + render::release_notes("Release v1.2.3", &changelog.entries, &changelog.invalid_entries); + + assert!(notes.contains("- Broken PR #42: missing `## Changelog` section")); + assert!(notes.contains("- Broken PR #43: parsing changelog TOML block:")); + } #[test] fn prerelease_uses_previous_prerelease() { diff --git a/xtask/src/changelog/render.rs b/xtask/src/changelog/render.rs index ca78f11981..98953cf012 100644 --- a/xtask/src/changelog/render.rs +++ b/xtask/src/changelog/render.rs @@ -1,6 +1,6 @@ use std::fmt::Write as _; -use super::{Impact, InvalidChangelogEntry, ReleaseNoteEntry, Scope}; +use super::{Impact, InvalidChangelogEntry, InvalidChangelogSource, ReleaseNoteEntry, Scope}; pub(super) fn release_notes( title: &str, @@ -52,10 +52,19 @@ fn append_invalid_entries(notes: &mut String, invalid_entries: &[InvalidChangelo .expect("writing to String cannot fail"); for entry in invalid_entries { - let pr_number = entry.pr_number; let reason = &entry.reason; - writeln!(notes, "- #{pr_number}: {reason}").expect("writing to String cannot fail"); + match &entry.source { + InvalidChangelogSource::PullRequest(pr_number) => { + writeln!(notes, "- Broken PR #{pr_number}: {reason}") + .expect("writing to String cannot fail"); + }, + InvalidChangelogSource::Commit(commit) => { + let abbreviated_commit = commit.chars().take(12).collect::(); + writeln!(notes, "- Missing PR for commit {abbreviated_commit}: {reason}") + .expect("writing to String cannot fail"); + }, + } } } @@ -173,7 +182,7 @@ mod tests { #[test] fn renders_callouts_and_changes_by_scope() { let invalid_entries = vec![InvalidChangelogEntry { - pr_number: 9, + source: InvalidChangelogSource::PullRequest(9), reason: "missing `## Changelog` section".to_owned(), order: 0, }]; @@ -216,7 +225,7 @@ mod tests { ## Changelog Entries Requiring Attention -- #9: missing `## Changelog` section +- Broken PR #9: missing `## Changelog` section ## Breaking Changes