From 0b059fb5e501d331dc688ee23fdc68581cf0bb0b Mon Sep 17 00:00:00 2001 From: ysaito1001 Date: Tue, 16 Jul 2024 11:14:16 -0500 Subject: [PATCH] Add support for changelog entry markdown files (#3763) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Motivation and Context [RFC: File-per-change changelog](https://smithy-lang.github.io/smithy-rs/design/rfcs/rfc0042_file_per_change_changelog.html#rfc-file-per-change-changelog) ## Description This PR is the first of the two implementing the proposal outlined in the RFC, and it focuses on the initial 4 bullets in the [Changes checklist](https://smithy-lang.github.io/smithy-rs/design/rfcs/rfc0042_file_per_change_changelog.html#changes-checklist). Crucially, this update does not modify the workflow for developers regarding changelog entries (editing `CHANGELOG.next.toml` remains necessary) or impact the `smithy-rs` CI process and our release pipeline. The PR introduces support for deserializing the following Markdown front matter in the YAML format described in the RFC, e.g. ``` --- # Adding `aws-sdk-rust` here duplicates this entry into the SDK changelog. applies_to: ["client", "server", "aws-sdk-rust"] authors: ["author1", "author2"] references: ["smithy-rs#1234", "aws-sdk-rust#1234"] # The previous `meta` section is broken up into its constituents: breaking: false # This replaces "tada": new_feature: false bug_fix: false --- Some message for the change. ``` These changelog entry Markdown files will be saved in the `smithy-rs/.changelog` directory: ``` ┌───────────┐ │ smithy-rs │ └─────┬─────┘ │ │ ┌───────────┐ ├────┤.changelog │ └─────┬─────┘ │ ├─────test1.md │ └─────test2.md ``` For a concrete example, see a test `end_to_end_changelog_entry_markdown_files` that directly converts from `end_to_end_changelog` and uses Markdown files (the expected test results remain the same). The next PR is expected to - add new subcommands to `changelogger` to a) create a Markdown file and b) preview changelog entries pending to be released - port existing `CHANGELOG.next.toml` at that time to individual Markdown files - update `sdk-lints` to disallow the existence of `CHANGELOG.next.toml` - pass a directory path for `smithy-rs/.changelog` to `--source` (one for [split](https://github.com/smithy-lang/smithy-rs/blob/dc970b37386b155eced5d41262ce0a7fc4a34a91/tools/ci-scripts/generate-smithy-rs-release#L22) and one for [render](https://github.com/smithy-lang/smithy-rs/blob/dc970b37386b155eced5d41262ce0a7fc4a34a91/tools/ci-scripts/generate-smithy-rs-release#L22)) and to [--source-to-truncate](https://github.com/smithy-lang/smithy-rs/blob/dc970b37386b155eced5d41262ce0a7fc4a34a91/tools/ci-scripts/generate-smithy-rs-release#L23C1-L23C47) (the 4th bullet effectively does the cutover to the new changelog mode) ## Testing - Added `parse_markdown` and `end_to_end_changelog_entry_markdown_files` for testing new changelog format (the rest of the tests showing up in the diff are moved from different places) - Confirmed via [a release dry-run](https://github.com/smithy-lang/smithy-rs/actions/runs/9947165056) that this PR does not break the existing `smithy-rs` release process - Confirmed that this PR does not break the use of `changelogger` in our internal release process ---- _By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice._ --- tools/ci-build/changelogger/Cargo.lock | 20 ++ tools/ci-build/changelogger/src/render.rs | 250 ++++++++++--- tools/ci-build/changelogger/src/split.rs | 16 +- tools/ci-build/sdk-lints/Cargo.lock | 20 ++ tools/ci-build/sdk-lints/src/changelog.rs | 9 +- .../ci-build/smithy-rs-tool-common/Cargo.toml | 1 + .../smithy-rs-tool-common/src/changelog.rs | 212 ++++------- .../src/changelog/parser.rs | 340 ++++++++++++++++++ 8 files changed, 671 insertions(+), 197 deletions(-) create mode 100644 tools/ci-build/smithy-rs-tool-common/src/changelog/parser.rs diff --git a/tools/ci-build/changelogger/Cargo.lock b/tools/ci-build/changelogger/Cargo.lock index 400851481d..f5b116dea6 100644 --- a/tools/ci-build/changelogger/Cargo.lock +++ b/tools/ci-build/changelogger/Cargo.lock @@ -997,6 +997,19 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_yaml" +version = "0.9.34+deprecated" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +dependencies = [ + "indexmap 2.2.6", + "itoa", + "ryu", + "serde", + "unsafe-libyaml", +] + [[package]] name = "slab" version = "0.4.9" @@ -1020,6 +1033,7 @@ dependencies = [ "semver", "serde", "serde_json", + "serde_yaml", "thiserror", "toml 0.5.11", "tracing", @@ -1329,6 +1343,12 @@ dependencies = [ "tinyvec", ] +[[package]] +name = "unsafe-libyaml" +version = "0.2.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" + [[package]] name = "url" version = "2.5.0" diff --git a/tools/ci-build/changelogger/src/render.rs b/tools/ci-build/changelogger/src/render.rs index 06781ab1c1..ef41109bba 100644 --- a/tools/ci-build/changelogger/src/render.rs +++ b/tools/ci-build/changelogger/src/render.rs @@ -10,12 +10,14 @@ use once_cell::sync::Lazy; use ordinal::Ordinal; use serde::Serialize; use smithy_rs_tool_common::changelog::{ - Changelog, HandAuthoredEntry, Reference, SdkModelChangeKind, SdkModelEntry, ValidationSet, + Changelog, ChangelogLoader, HandAuthoredEntry, Reference, SdkModelChangeKind, SdkModelEntry, + ValidationSet, }; use smithy_rs_tool_common::git::{find_git_repository_root, Git, GitCLI}; use smithy_rs_tool_common::versions_manifest::{CrateVersionMetadataMap, VersionsManifest}; use std::env; use std::fmt::Write; +use std::fs; use std::path::{Path, PathBuf}; use time::OffsetDateTime; @@ -244,9 +246,14 @@ fn render_table_row(columns: [&str; 2], out: &mut String) { fn load_changelogs(args: &RenderArgs) -> Result { let mut combined = Changelog::new(); + let loader = ChangelogLoader::default(); for source in &args.source { - let changelog = Changelog::load_from_file(source) - .map_err(|errs| anyhow::Error::msg(format!("failed to load {source:?}: {errs:#?}")))?; + let changelog = if source.is_dir() { + loader.load_from_dir(source) + } else { + loader.load_from_file(source) + } + .map_err(|errs| anyhow::Error::msg(format!("failed to load {source:?}: {errs:#?}")))?; changelog.validate(ValidationSet::Render).map_err(|errs| { anyhow::Error::msg(format!( "failed to load {source:?}: {errors}", @@ -321,8 +328,14 @@ fn update_changelogs( update.push_str(¤t); std::fs::write(&args.changelog_output, update).context("failed to write rendered changelog")?; - std::fs::write(&args.source_to_truncate, EXAMPLE_ENTRY.trim()) - .context("failed to truncate source")?; + if args.source_to_truncate.is_dir() { + fs::remove_dir_all(&args.source_to_truncate) + .and_then(|_| fs::create_dir(&args.source_to_truncate)) + .with_context(|| format!("failed to empty directory {:?}", &args.source_to_truncate))?; + } else { + fs::write(&args.source_to_truncate, EXAMPLE_ENTRY.trim()) + .with_context(|| format!("failed to truncate source {:?}", &args.source_to_truncate))?; + } eprintln!("Changelogs updated!"); Ok(()) } @@ -494,11 +507,14 @@ fn render( #[cfg(test)] mod test { use super::{date_based_release_metadata, render, Changelog, ChangelogEntries, ChangelogEntry}; + use smithy_rs_tool_common::changelog::ChangelogLoader; use smithy_rs_tool_common::{ changelog::SdkAffected, package::PackageCategory, versions_manifest::{CrateVersion, CrateVersionMetadataMap}, }; + use std::fs; + use tempfile::TempDir; use time::OffsetDateTime; fn render_full(entries: &[ChangelogEntry], release_header: &str) -> String { @@ -506,6 +522,174 @@ mod test { format!("{header}{body}") } + const SMITHY_RS_EXPECTED_END_TO_END: &str = r#"v0.3.0 (January 4th, 2022) +========================== +**Breaking Changes:** +- :warning: (all, [smithy-rs#445](https://github.com/smithy-lang/smithy-rs/issues/445)) I made a major change to update the code generator + +**New this release:** +- :tada: (all, [smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [aws-sdk#123](https://github.com/aws/aws-sdk/issues/123), @external-contrib, @other-external-dev) I made a change to update the code generator +- :tada: (all, [smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [smithy-rs#447](https://github.com/smithy-lang/smithy-rs/issues/447), @external-contrib, @other-external-dev) I made a change to update the code generator + + **Update guide:** + blah blah +- (all, [smithy-rs#200](https://github.com/smithy-lang/smithy-rs/issues/200), @another-contrib) I made a minor change + +**Contributors** +Thank you for your contributions! ❤ +- @another-contrib ([smithy-rs#200](https://github.com/smithy-lang/smithy-rs/issues/200)) +- @external-contrib ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [smithy-rs#447](https://github.com/smithy-lang/smithy-rs/issues/447)) +- @other-external-dev ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [smithy-rs#447](https://github.com/smithy-lang/smithy-rs/issues/447)) + +"#; + + const AWS_SDK_EXPECTED_END_TO_END: &str = r#"v0.1.0 (January 4th, 2022) +========================== +**Breaking Changes:** +- :warning: ([smithy-rs#445](https://github.com/smithy-lang/smithy-rs/issues/445)) I made a major change to update the AWS SDK + +**New this release:** +- :tada: ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), @external-contrib) I made a change to update the code generator + +**Service Features:** +- `aws-sdk-ec2` (0.12.0): Some API change +- `aws-sdk-s3` (0.14.0): Some new API to do X + +**Service Documentation:** +- `aws-sdk-ec2` (0.12.0): Updated some docs + +**Contributors** +Thank you for your contributions! ❤ +- @external-contrib ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446)) + +"#; + + #[test] + fn end_to_end_changelog_entry_markdown_files() { + let temp_dir = TempDir::new().unwrap(); + let smithy_rs_entry1 = r#"--- +applies_to: ["client", "server"] +authors: ["rcoh", "jdisanti"] +references: ["smithy-rs#445"] +breaking: true +new_feature: false +bug_fix: false +--- +I made a major change to update the code generator +"#; + let smithy_rs_entry2 = r#"--- +applies_to: ["client", "server"] +authors: ["external-contrib", "other-external-dev"] +references: ["smithy-rs#446", "aws-sdk#123"] +breaking: false +new_feature: true +bug_fix: false +--- +I made a change to update the code generator +"#; + let smithy_rs_entry3 = r#"--- +applies_to: ["client", "server"] +authors: ["another-contrib"] +references: ["smithy-rs#200"] +breaking: false +new_feature: false +bug_fix: false +--- +I made a minor change +"#; + let aws_sdk_entry1 = r#"--- +applies_to: ["aws-sdk-rust"] +authors: ["rcoh"] +references: ["smithy-rs#445"] +breaking: true +new_feature: false +bug_fix: false +--- +I made a major change to update the AWS SDK +"#; + let aws_sdk_entry2 = r#"--- +applies_to: ["aws-sdk-rust"] +authors: ["external-contrib"] +references: ["smithy-rs#446"] +breaking: false +new_feature: true +bug_fix: false +--- +I made a change to update the code generator +"#; + let smithy_rs_entry4 = r#"--- +applies_to: ["client", "server"] +authors: ["external-contrib", "other-external-dev"] +references: ["smithy-rs#446", "smithy-rs#447"] +breaking: false +new_feature: true +bug_fix: false +--- +I made a change to update the code generator + +**Update guide:** +blah blah +"#; + + // We won't handwrite changelog entries for model updates, and they are still provided in + // the TOML format. + let model_updates = r#" +[[aws-sdk-model]] +module = "aws-sdk-s3" +version = "0.14.0" +kind = "Feature" +message = "Some new API to do X" + +[[aws-sdk-model]] +module = "aws-sdk-ec2" +version = "0.12.0" +kind = "Documentation" +message = "Updated some docs" + +[[aws-sdk-model]] +module = "aws-sdk-ec2" +version = "0.12.0" +kind = "Feature" +message = "Some API change" + "#; + + [ + smithy_rs_entry1, + smithy_rs_entry2, + smithy_rs_entry3, + aws_sdk_entry1, + aws_sdk_entry2, + smithy_rs_entry4, + ] + .iter() + .enumerate() + .for_each(|(i, contents)| { + let changelog_entry_markdown_file = temp_dir.path().join(format!("test{i}.md")); + fs::write(&changelog_entry_markdown_file, contents.as_bytes()).unwrap(); + }); + + let mut changelog = ChangelogLoader::default() + .load_from_dir(temp_dir.path()) + .expect("valid changelog"); + + changelog.merge( + ChangelogLoader::default() + .parse_str(model_updates) + .expect("valid changelog"), + ); + + let ChangelogEntries { + aws_sdk_rust, + smithy_rs, + } = changelog.into(); + + let smithy_rs_rendered = render_full(&smithy_rs, "v0.3.0 (January 4th, 2022)"); + pretty_assertions::assert_str_eq!(SMITHY_RS_EXPECTED_END_TO_END, smithy_rs_rendered); + + let aws_sdk_rendered = render_full(&aws_sdk_rust, "v0.1.0 (January 4th, 2022)"); + pretty_assertions::assert_str_eq!(AWS_SDK_EXPECTED_END_TO_END, aws_sdk_rendered); + } + #[test] fn end_to_end_changelog() { let changelog_toml = r#" @@ -568,61 +752,19 @@ version = "0.12.0" kind = "Feature" message = "Some API change" "#; - let changelog: Changelog = Changelog::parse_str(changelog_toml).expect("valid changelog"); + let changelog: Changelog = ChangelogLoader::default() + .parse_str(changelog_toml) + .expect("valid changelog"); let ChangelogEntries { aws_sdk_rust, smithy_rs, } = changelog.into(); let smithy_rs_rendered = render_full(&smithy_rs, "v0.3.0 (January 4th, 2022)"); - let smithy_rs_expected = r#" -v0.3.0 (January 4th, 2022) -========================== -**Breaking Changes:** -- :warning: (all, [smithy-rs#445](https://github.com/smithy-lang/smithy-rs/issues/445)) I made a major change to update the code generator - -**New this release:** -- :tada: (all, [smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [aws-sdk#123](https://github.com/aws/aws-sdk/issues/123), @external-contrib, @other-external-dev) I made a change to update the code generator -- :tada: (all, [smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [smithy-rs#447](https://github.com/smithy-lang/smithy-rs/issues/447), @external-contrib, @other-external-dev) I made a change to update the code generator - - **Update guide:** - blah blah -- (all, [smithy-rs#200](https://github.com/smithy-lang/smithy-rs/issues/200), @another-contrib) I made a minor change - -**Contributors** -Thank you for your contributions! ❤ -- @another-contrib ([smithy-rs#200](https://github.com/smithy-lang/smithy-rs/issues/200)) -- @external-contrib ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [smithy-rs#447](https://github.com/smithy-lang/smithy-rs/issues/447)) -- @other-external-dev ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), [smithy-rs#447](https://github.com/smithy-lang/smithy-rs/issues/447)) - -"# - .trim_start(); - pretty_assertions::assert_str_eq!(smithy_rs_expected, smithy_rs_rendered); + pretty_assertions::assert_str_eq!(SMITHY_RS_EXPECTED_END_TO_END, smithy_rs_rendered); let aws_sdk_rust_rendered = render_full(&aws_sdk_rust, "v0.1.0 (January 4th, 2022)"); - let aws_sdk_expected = r#" -v0.1.0 (January 4th, 2022) -========================== -**Breaking Changes:** -- :warning: ([smithy-rs#445](https://github.com/smithy-lang/smithy-rs/issues/445)) I made a major change to update the AWS SDK - -**New this release:** -- :tada: ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446), @external-contrib) I made a change to update the code generator - -**Service Features:** -- `aws-sdk-ec2` (0.12.0): Some API change -- `aws-sdk-s3` (0.14.0): Some new API to do X - -**Service Documentation:** -- `aws-sdk-ec2` (0.12.0): Updated some docs - -**Contributors** -Thank you for your contributions! ❤ -- @external-contrib ([smithy-rs#446](https://github.com/smithy-lang/smithy-rs/issues/446)) - -"# - .trim_start(); - pretty_assertions::assert_str_eq!(aws_sdk_expected, aws_sdk_rust_rendered); + pretty_assertions::assert_str_eq!(AWS_SDK_EXPECTED_END_TO_END, aws_sdk_rust_rendered); } #[test] @@ -670,7 +812,9 @@ meta = { breaking = false, tada = true, bug = false } references = ["smithy-rs#446"] author = "rcoh" "#; - let changelog: Changelog = Changelog::parse_str(sample).expect("valid changelog"); + let changelog: Changelog = ChangelogLoader::default() + .parse_str(sample) + .expect("valid changelog"); let ChangelogEntries { aws_sdk_rust: _, smithy_rs, diff --git a/tools/ci-build/changelogger/src/split.rs b/tools/ci-build/changelogger/src/split.rs index cd76496c96..7b9d034d18 100644 --- a/tools/ci-build/changelogger/src/split.rs +++ b/tools/ci-build/changelogger/src/split.rs @@ -5,7 +5,7 @@ use anyhow::{Context, Result}; use clap::Parser; -use smithy_rs_tool_common::changelog::Changelog; +use smithy_rs_tool_common::changelog::{Changelog, ChangelogLoader}; use smithy_rs_tool_common::git::{find_git_repository_root, Git, GitCLI}; use std::path::{Path, PathBuf}; use std::{env, fs, mem}; @@ -14,6 +14,8 @@ use std::{env, fs, mem}; // SDK changelog entries, but small enough that the SDK changelog file // doesn't get too long. const MAX_ENTRY_AGE: usize = 5; +// TODO(file-per-change-changelog): Remove `INTERMEDIATE_SOURCE_HEADER` once we have switched over +// to the new markdown format. const INTERMEDIATE_SOURCE_HEADER: &str = "# This is an intermediate file that will be replaced after automation is complete.\n\ # It will be used to generate a changelog entry for smithy-rs.\n\ @@ -42,14 +44,20 @@ pub struct SplitArgs { } pub fn subcommand_split(args: &SplitArgs) -> Result<()> { - let combined_changelog = Changelog::load_from_file(&args.source).map_err(|errs| { + let loader = ChangelogLoader::default(); + let combined_changelog = if args.source.is_dir() { + loader.load_from_dir(&args.source) + } else { + loader.load_from_file(&args.source) + } + .map_err(|errs| { anyhow::Error::msg(format!( "cannot split changelogs with changelog errors: {:#?}", errs )) })?; let current_sdk_changelog = if args.destination.exists() { - Changelog::load_from_file(&args.destination).map_err(|errs| { + loader.load_from_file(&args.destination).map_err(|errs| { anyhow::Error::msg(format!( "failed to load existing SDK changelog entries: {:#?}", errs @@ -65,6 +73,8 @@ pub fn subcommand_split(args: &SplitArgs) -> Result<()> { ); let sdk_changelog = merge_sdk_entries(current_sdk_changelog, new_sdk_entries); + // TODO(file-per-change-changelog): Remove writing to `INTERMEDIATE_SOURCE_HEADER` once we have + // switched over to the new markdown format. write_entries(&args.source, INTERMEDIATE_SOURCE_HEADER, &smithy_rs_entries) .context("failed to write source")?; write_entries(&args.destination, DEST_HEADER, &sdk_changelog) diff --git a/tools/ci-build/sdk-lints/Cargo.lock b/tools/ci-build/sdk-lints/Cargo.lock index d4ed8832b7..b08323929b 100644 --- a/tools/ci-build/sdk-lints/Cargo.lock +++ b/tools/ci-build/sdk-lints/Cargo.lock @@ -930,6 +930,19 @@ dependencies = [ "serde", ] +[[package]] +name = "serde_yaml" +version = "0.9.34+deprecated" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6a8b1a1a2ebf674015cc02edccce75287f1a0130d394307b36743c2f5d504b47" +dependencies = [ + "indexmap 2.2.6", + "itoa", + "ryu", + "serde", + "unsafe-libyaml", +] + [[package]] name = "slab" version = "0.4.9" @@ -953,6 +966,7 @@ dependencies = [ "semver", "serde", "serde_json", + "serde_yaml", "thiserror", "toml 0.5.11", "tracing", @@ -1241,6 +1255,12 @@ dependencies = [ "tinyvec", ] +[[package]] +name = "unsafe-libyaml" +version = "0.2.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "673aac59facbab8a9007c7f6108d11f63b603f7cabff99fabf650fea5c32b861" + [[package]] name = "url" version = "2.5.0" diff --git a/tools/ci-build/sdk-lints/src/changelog.rs b/tools/ci-build/sdk-lints/src/changelog.rs index 03a04a2d4e..e3144948bb 100644 --- a/tools/ci-build/sdk-lints/src/changelog.rs +++ b/tools/ci-build/sdk-lints/src/changelog.rs @@ -6,7 +6,7 @@ use crate::lint::LintError; use crate::{repo_root, Check, Lint}; use anyhow::Result; -use smithy_rs_tool_common::changelog::{Changelog, ValidationSet}; +use smithy_rs_tool_common::changelog::{ChangelogLoader, ValidationSet}; use std::path::{Path, PathBuf}; pub(crate) struct ChangelogNext; @@ -30,9 +30,14 @@ impl Check for ChangelogNext { } } +// TODO(file-per-change-changelog): Use `.load_from_dir` to read from the `.changelog` directory +// and run the validation only when the directory has at least one changelog entry file, otherwise +// a default constructed `ChangeLog` won't pass the validation. /// Validate that `CHANGELOG.next.toml` follows best practices fn check_changelog_next(path: impl AsRef) -> std::result::Result<(), Vec> { - let parsed = Changelog::load_from_file(path).map_err(|e| vec![LintError::via_display(e)])?; + let parsed = ChangelogLoader::default() + .load_from_file(path) + .map_err(|e| vec![LintError::via_display(e)])?; parsed .validate(ValidationSet::Development) .map_err(|errs| { diff --git a/tools/ci-build/smithy-rs-tool-common/Cargo.toml b/tools/ci-build/smithy-rs-tool-common/Cargo.toml index d9fd0093e7..d782a64df7 100644 --- a/tools/ci-build/smithy-rs-tool-common/Cargo.toml +++ b/tools/ci-build/smithy-rs-tool-common/Cargo.toml @@ -26,6 +26,7 @@ reqwest = { version = "0.11.10", features = ["blocking"] } semver = "1" serde = { version = "1", features = ["derive"] } serde_json = "1" +serde_yaml = "0.9" thiserror = "1.0.56" tokio = { version = "1.20.1", features = ["rt", "macros"], optional = true } toml = { version = "0.5.8", features = ["preserve_order"] } diff --git a/tools/ci-build/smithy-rs-tool-common/src/changelog.rs b/tools/ci-build/smithy-rs-tool-common/src/changelog.rs index 021016557f..0be4cfd183 100644 --- a/tools/ci-build/smithy-rs-tool-common/src/changelog.rs +++ b/tools/ci-build/smithy-rs-tool-common/src/changelog.rs @@ -5,9 +5,12 @@ //! This module holds deserializable structs for the hand-authored changelog TOML files used in smithy-rs. +pub mod parser; + +use crate::changelog::parser::{ParseIntoChangelog, ParserChain}; use anyhow::{bail, Context, Result}; use serde::{de, Deserialize, Deserializer, Serialize}; -use std::fmt::{self, Display}; +use std::fmt; use std::path::Path; use std::str::FromStr; @@ -22,7 +25,7 @@ pub enum SdkAffected { All, } -impl Display for SdkAffected { +impl fmt::Display for SdkAffected { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { SdkAffected::Client => write!(f, "client"), @@ -277,42 +280,6 @@ impl Changelog { self.sdk_models.extend(other.sdk_models); } - pub fn parse_str(value: &str) -> Result { - let mut changelog: Changelog = - (match toml::from_str(value).context("Invalid TOML changelog format") { - Ok(parsed) => Ok(parsed), - Err(toml_err) => { - // Remove comments from the top - let value = value - .split('\n') - .filter(|line| !line.trim().starts_with('#')) - .collect::>() - .join("\n"); - match serde_json::from_str(&value).context("Invalid JSON changelog format") { - Ok(parsed) => Ok(parsed), - Err(json_err) => bail!( - "Invalid JSON or TOML changelog format:\n{:?}\n{:?}", - toml_err, - json_err - ), - } - } - } as Result)?; - // all smithry-rs entries should have meta.target set to the default value instead of None - for entry in &mut changelog.smithy_rs { - if entry.meta.target.is_none() { - entry.meta.target = Some(SdkAffected::default()); - } - } - Ok(changelog) - } - - pub fn load_from_file(path: impl AsRef) -> Result { - let contents = std::fs::read_to_string(path.as_ref()) - .with_context(|| format!("failed to read {:?}", path.as_ref()))?; - Self::parse_str(&contents) - } - pub fn to_json_string(&self) -> Result { serde_json::to_string_pretty(self).context("failed to serialize changelog JSON") } @@ -350,57 +317,66 @@ impl Changelog { } } +/// Parses changelog entries into [`Changelog`] using a series of parsers. +/// +/// Each parser will attempt to parse an input string in order: +/// * If a parser successfully parses the input string into `Changelog`, it will be returned immediately. +/// No other parsers will be used. +/// * Otherwise, if a parser returns an `anyhow::Error`, the next parser will be tried. +/// * If none of the parsers parse the input string successfully, an error will be returned from the chain. +#[derive(Debug, Default)] +pub struct ChangelogLoader { + parser_chain: ParserChain, +} + +impl ChangelogLoader { + /// Parses the given `value` into a `Changelog` + pub fn parse_str(&self, value: impl AsRef) -> Result { + self.parser_chain.parse(value.as_ref()) + } + + /// Parses the contents of a file located at `path` into `Changelog` + pub fn load_from_file(&self, path: impl AsRef) -> Result { + let contents = std::fs::read_to_string(path.as_ref()) + .with_context(|| format!("failed to read {:?}", path.as_ref()))?; + self.parse_str(contents) + } + + /// Parses the contents of files stored in a directory `dir_path` into `Changelog` + /// + /// It opens each file in the directory, parses the file contents into `Changelog`, + /// and merges it with accumulated `Changelog`. It currently does not support loading + /// from recursive directory structures. + pub fn load_from_dir(&self, dir_path: impl AsRef) -> Result { + let entries = std::fs::read_dir(dir_path.as_ref())?; + let result = entries + .into_iter() + .filter_map(|entry| { + // Convert each entry to its path if it's a file + entry.ok().and_then(|entry| { + let path = entry.path(); + if path.is_file() { + Some(path) + } else { + None + } + }) + }) + .try_fold(Changelog::new(), |mut combined_changelog, path| { + combined_changelog.merge(self.load_from_file(path)?); + Ok::<_, anyhow::Error>(combined_changelog) + })?; + + Ok(result) + } +} + #[cfg(test)] mod tests { use super::*; + use crate::changelog::parser::Toml; use anyhow::Context; - #[test] - fn parse_json() { - let json = r#" - # Example changelog entries - # [[aws-sdk-rust]] - # message = "Fix typos in module documentation for generated crates" - # references = ["smithy-rs#920"] - # meta = { "breaking" = false, "tada" = false, "bug" = false } - # author = "rcoh" - # - # [[smithy-rs]] - # message = "Fix typos in module documentation for generated crates" - # references = ["smithy-rs#920"] - # meta = { "breaking" = false, "tada" = false, "bug" = false } - # author = "rcoh" - { - "smithy-rs": [], - "aws-sdk-rust": [ - { - "message": "Some change", - "meta": { "bug": true, "breaking": false, "tada": false }, - "author": "test-dev", - "references": [ - "aws-sdk-rust#123", - "smithy-rs#456" - ] - } - ], - "aws-sdk-model": [ - { - "module": "aws-sdk-ec2", - "version": "0.12.0", - "kind": "Feature", - "message": "Some API change" - } - ] - } - "#; - let changelog = Changelog::parse_str(json).unwrap(); - assert!(changelog.smithy_rs.is_empty()); - assert_eq!(1, changelog.aws_sdk_rust.len()); - assert_eq!("Some change", changelog.aws_sdk_rust[0].message); - assert_eq!(1, changelog.sdk_models.len()); - assert_eq!("Some API change", changelog.sdk_models[0].message); - } - #[test] fn errors_are_combined() { let buffer = r#" @@ -425,11 +401,13 @@ mod tests { let res = changelog.validate(ValidationSet::Development); assert!(res.is_err()); if let Err(e) = res { - assert_eq!(e.len(), 3); + assert_eq!(3, e.len()); assert!(e.contains(&"smithy-rs entry must have an affected target".to_string())) } } + // TODO(file-per-change-changelog): Remove this test once we have switched to the new markdown + // format because targets will be explicit and there won't be defaults set. #[test] fn confirm_smithy_rs_defaults_set() { let buffer = r#" @@ -450,7 +428,7 @@ mod tests { author = "fz" "#; { - // loading directly from toml::from_str won't set the default target field + // parsing directly using `toml::from_str` won't set the default target field let changelog: Changelog = toml::from_str(buffer).expect("valid changelog"); let res = changelog.validate(ValidationSet::Development); assert!(res.is_err()); @@ -459,8 +437,8 @@ mod tests { } } { - // loading through Chanelog will result in no error - let changelog: Changelog = Changelog::parse_str(buffer).expect("valid changelog"); + // parsing through the `Toml` parser will result in no error + let changelog: Changelog = Toml::default().parse(buffer).expect("valid changelog"); let res = changelog.validate(ValidationSet::Development); assert!(res.is_ok()); if let Err(e) = res { @@ -470,50 +448,6 @@ mod tests { } } - #[test] - fn parse_smithy_ok() { - // by default smithy-rs meta data should say change is for both - let toml = r#" - [[aws-sdk-rust]] - message = "Fix typos in module documentation for generated crates" - references = ["smithy-rs#920"] - meta = { "breaking" = false, "tada" = false, "bug" = false } - author = "rcoh" - [[smithy-rs]] - message = "Fix typos in module documentation for generated crates" - references = ["smithy-rs#920"] - meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" } - author = "rcoh" - [[smithy-rs]] - message = "Fix typos in module documentation for generated crates" - references = ["smithy-rs#920"] - meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server" } - author = "rcoh" - [[smithy-rs]] - message = "Fix typos in module documentation for generated crates" - references = ["smithy-rs#920"] - meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "all" } - author = "rcoh" - [[smithy-rs]] - message = "Fix typos in module documentation for generated crates" - references = ["smithy-rs#920"] - meta = { "breaking" = false, "tada" = false, "bug" = false } - author = "rcoh" - "#; - let changelog = Changelog::parse_str(toml).unwrap(); - assert_eq!(changelog.smithy_rs.len(), 4); - assert_eq!( - changelog.smithy_rs[0].meta.target, - Some(SdkAffected::Client) - ); - assert_eq!( - changelog.smithy_rs[1].meta.target, - Some(SdkAffected::Server) - ); - assert_eq!(changelog.smithy_rs[2].meta.target, Some(SdkAffected::All)); - assert_eq!(changelog.smithy_rs[3].meta.target, Some(SdkAffected::All)); - } - #[test] fn test_hand_authored_sdk() { // server target @@ -527,7 +461,7 @@ mod tests { let value: HandAuthoredEntry = toml::from_str(value) .context("String should have parsed") .unwrap(); - assert_eq!(value.meta.target, Some(SdkAffected::Server)); + assert_eq!(Some(SdkAffected::Server), value.meta.target); } // client target @@ -541,7 +475,7 @@ mod tests { let value: HandAuthoredEntry = toml::from_str(value) .context("String should have parsed") .unwrap(); - assert_eq!(value.meta.target, Some(SdkAffected::Client)); + assert_eq!(Some(SdkAffected::Client), value.meta.target); } // Both target let value = r#" @@ -554,7 +488,7 @@ mod tests { let value: HandAuthoredEntry = toml::from_str(value) .context("String should have parsed") .unwrap(); - assert_eq!(value.meta.target, Some(SdkAffected::All)); + assert_eq!(Some(SdkAffected::All), value.meta.target); } // an invalid sdk value let value = r#" @@ -579,7 +513,7 @@ mod tests { let value: HandAuthoredEntry = toml::from_str(value) .context("String should have parsed as it has none meta.sdk") .unwrap(); - assert_eq!(value.meta.target, None); + assert_eq!(None, value.meta.target); } // single author let value = r#" @@ -592,7 +526,7 @@ mod tests { let value: HandAuthoredEntry = toml::from_str(value) .context("String should have parsed with multiple authors") .unwrap(); - assert_eq!(value.authors, Authors(vec!["rcoh".to_string()])); + assert_eq!(Authors(vec!["rcoh".to_string()]), value.authors); } // multiple authors let value = r#" @@ -606,8 +540,8 @@ mod tests { .context("String should have parsed with multiple authors") .unwrap(); assert_eq!( - value.authors, - Authors(vec!["rcoh".to_string(), "crisidev".to_string()]) + Authors(vec!["rcoh".to_string(), "crisidev".to_string()]), + value.authors ); } } diff --git a/tools/ci-build/smithy-rs-tool-common/src/changelog/parser.rs b/tools/ci-build/smithy-rs-tool-common/src/changelog/parser.rs new file mode 100644 index 0000000000..27af7ba63e --- /dev/null +++ b/tools/ci-build/smithy-rs-tool-common/src/changelog/parser.rs @@ -0,0 +1,340 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +use crate::changelog::{Authors, Changelog, HandAuthoredEntry, Meta, Reference, SdkAffected}; +use anyhow::{bail, Context, Result}; +use serde::{Deserialize, Serialize}; +use std::collections::HashSet; +use std::fmt::Debug; + +pub(crate) trait ParseIntoChangelog: Debug { + fn parse(&self, value: &str) -> Result; +} + +#[derive(Clone, Debug, Default)] +pub(super) struct Toml; +impl ParseIntoChangelog for Toml { + fn parse(&self, value: &str) -> Result { + let mut changelog: Changelog = + toml::from_str(value).context("Invalid TOML changelog format")?; + // all smithry-rs entries should have meta.target set to the default value instead of None + // TODO(file-per-change-changelog): Remove the following fix-up once we have switched over + // to the new markdown format since it won't be needed. + for entry in &mut changelog.smithy_rs { + if entry.meta.target.is_none() { + entry.meta.target = Some(SdkAffected::default()); + } + } + Ok(changelog) + } +} + +#[derive(Clone, Debug, Default)] +struct Json; +impl ParseIntoChangelog for Json { + fn parse(&self, value: &str) -> Result { + // Remove comments from the top + let value = value + .split('\n') + .filter(|line| !line.trim().starts_with('#')) + .collect::>() + .join("\n"); + serde_json::from_str(&value).context("Invalid JSON changelog format") + } +} + +#[derive(Copy, Clone, Debug, Deserialize, Hash, Serialize, PartialEq, Eq)] +enum Target { + #[serde(rename = "client")] + Client, + #[serde(rename = "server")] + Server, + #[serde(rename = "aws-sdk-rust")] + AwsSdk, +} + +#[derive(Clone, Debug, Default, Deserialize)] +struct FrontMatter { + applies_to: HashSet, + authors: Authors, + references: Vec, + breaking: bool, + new_feature: bool, + bug_fix: bool, +} + +#[derive(Clone, Debug, Default)] +struct Markdown { + front_matter: FrontMatter, + message: String, +} + +impl From for Changelog { + fn from(value: Markdown) -> Self { + let front_matter = value.front_matter; + let entry = HandAuthoredEntry { + message: value.message.trim_start().to_owned(), + meta: Meta { + bug: front_matter.bug_fix, + breaking: front_matter.breaking, + tada: front_matter.new_feature, + target: None, + }, + authors: front_matter.authors, + references: front_matter.references, + since_commit: None, + age: None, + }; + + let mut changelog = Changelog::new(); + + // Bin `entry` into the appropriate `Vec` based on the `applies_to` field in `front_matter` + if front_matter.applies_to.contains(&Target::AwsSdk) { + changelog.aws_sdk_rust.push(entry.clone()) + } + if front_matter.applies_to.contains(&Target::Client) + && front_matter.applies_to.contains(&Target::Server) + { + let mut entry = entry.clone(); + entry.meta.target = Some(SdkAffected::All); + changelog.smithy_rs.push(entry); + } else if front_matter.applies_to.contains(&Target::Client) { + let mut entry = entry.clone(); + entry.meta.target = Some(SdkAffected::Client); + changelog.smithy_rs.push(entry); + } else if front_matter.applies_to.contains(&Target::Server) { + let mut entry = entry.clone(); + entry.meta.target = Some(SdkAffected::Server); + changelog.smithy_rs.push(entry); + } + + changelog + } +} + +impl ParseIntoChangelog for Markdown { + fn parse(&self, value: &str) -> Result { + let mut parts = value.splitn(3, "---"); + let _ = parts.next(); // Skip first empty element + let front_matter_str = parts + .next() + .context("front matter should follow the opening `---`")?; + let message = parts + .next() + .context("message should be included in changelog entry")?; + + let markdown = Markdown { + front_matter: serde_yaml::from_str(front_matter_str)?, + message: message.to_owned(), + }; + + Ok(markdown.into()) + } +} + +#[derive(Debug)] +pub(crate) struct ParserChain { + parsers: Vec<(&'static str, Box)>, +} + +impl Default for ParserChain { + fn default() -> Self { + Self { + parsers: vec![ + ("markdown", Box::::default()), + ("toml", Box::::default()), + ("json", Box::::default()), + ], + } + } +} + +impl ParseIntoChangelog for ParserChain { + fn parse(&self, value: &str) -> Result { + for (name, parser) in &self.parsers { + match parser.parse(value) { + Ok(parsed) => { + return Ok(parsed); + } + Err(err) => { + tracing::debug!(parser = %name, err = %err, "failed to parse the input string"); + } + } + } + bail!("no parsers in chain parsed ${value} into `Changelog`") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_json() { + let json = r#" + # Example changelog entries + # [[aws-sdk-rust]] + # message = "Fix typos in module documentation for generated crates" + # references = ["smithy-rs#920"] + # meta = { "breaking" = false, "tada" = false, "bug" = false } + # author = "rcoh" + # + # [[smithy-rs]] + # message = "Fix typos in module documentation for generated crates" + # references = ["smithy-rs#920"] + # meta = { "breaking" = false, "tada" = false, "bug" = false } + # author = "rcoh" + { + "smithy-rs": [], + "aws-sdk-rust": [ + { + "message": "Some change", + "meta": { "bug": true, "breaking": false, "tada": false }, + "author": "test-dev", + "references": [ + "aws-sdk-rust#123", + "smithy-rs#456" + ] + } + ], + "aws-sdk-model": [ + { + "module": "aws-sdk-ec2", + "version": "0.12.0", + "kind": "Feature", + "message": "Some API change" + } + ] + } + "#; + let changelog = Json::default().parse(json).unwrap(); + assert!(changelog.smithy_rs.is_empty()); + assert_eq!(1, changelog.aws_sdk_rust.len()); + assert_eq!("Some change", changelog.aws_sdk_rust[0].message); + assert_eq!(1, changelog.sdk_models.len()); + assert_eq!("Some API change", changelog.sdk_models[0].message); + } + + #[test] + fn parse_toml() { + // TODO(file-per-change-changelog): We keep the following test string while transitioning + // to the new markdown format. Once we have switched to the new format, only use + // `[[aws-sdk-model]]` in the test string because after the cutover, `[[aws-sdk-rust]]` or + // `[[smithy-rs]]` are not a recommended way of writing changelogs. + let toml = r#" + [[aws-sdk-rust]] + message = "Fix typos in module documentation for generated crates" + references = ["smithy-rs#920"] + meta = { "breaking" = false, "tada" = false, "bug" = false } + author = "rcoh" + [[smithy-rs]] + message = "Fix typos in module documentation for generated crates" + references = ["smithy-rs#920"] + meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client" } + author = "rcoh" + [[smithy-rs]] + message = "Fix typos in module documentation for generated crates" + references = ["smithy-rs#920"] + meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server" } + author = "rcoh" + [[smithy-rs]] + message = "Fix typos in module documentation for generated crates" + references = ["smithy-rs#920"] + meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "all" } + author = "rcoh" + [[smithy-rs]] + message = "Fix typos in module documentation for generated crates" + references = ["smithy-rs#920"] + meta = { "breaking" = false, "tada" = false, "bug" = false } + author = "rcoh" + "#; + let changelog = Toml::default().parse(toml).unwrap(); + assert_eq!(4, changelog.smithy_rs.len()); + assert_eq!( + Some(SdkAffected::Client), + changelog.smithy_rs[0].meta.target, + ); + assert_eq!( + Some(SdkAffected::Server), + changelog.smithy_rs[1].meta.target, + ); + assert_eq!(Some(SdkAffected::All), changelog.smithy_rs[2].meta.target); + assert_eq!(Some(SdkAffected::All), changelog.smithy_rs[3].meta.target); + } + + #[test] + fn parse_markdown() { + { + let markdown = r#"--- +applies_to: ["client", "server", "aws-sdk-rust"] +authors: ["landonxjames","todaaron"] +references: ["smithy-rs#123"] +breaking: false +new_feature: false +bug_fix: false +--- +# Markdown Content +This is some **Markdown** content. +"#; + let changelog = Markdown::default().parse(markdown).unwrap(); + assert_eq!(1, changelog.smithy_rs.len()); + assert_eq!(Some(SdkAffected::All), changelog.smithy_rs[0].meta.target); + assert_eq!( + "# Markdown Content\nThis is some **Markdown** content.\n", + &changelog.smithy_rs[0].message + ); + // Should duplicate this entry into the SDK changelog by virtue of `aws-sdk-rust` + assert_eq!(1, changelog.aws_sdk_rust.len()); + } + { + let markdown = r#"--- +applies_to: ["client"] +authors: ["velfi"] +references: ["smithy-rs#456", "aws-sdk-rust#1234"] +breaking: false +new_feature: false +bug_fix: false +--- +# Markdown Content +This is some **Markdown** content. +"#; + let changelog = Markdown::default().parse(markdown).unwrap(); + assert_eq!(1, changelog.smithy_rs.len()); + assert_eq!( + Some(SdkAffected::Client), + changelog.smithy_rs[0].meta.target + ); + assert_eq!( + "# Markdown Content\nThis is some **Markdown** content.\n", + &changelog.smithy_rs[0].message + ); + assert!(changelog.aws_sdk_rust.is_empty()); + } + { + let markdown = r#"--- +applies_to: ["server"] +authors: ["david-perez", "drganjoo"] +references: ["smithy-rs#789"] +breaking: false +new_feature: false +bug_fix: true +--- +# Markdown Content +This is some **Markdown** content. +"#; + let changelog = Markdown::default().parse(markdown).unwrap(); + assert_eq!(1, changelog.smithy_rs.len()); + assert_eq!( + Some(SdkAffected::Server), + changelog.smithy_rs[0].meta.target + ); + assert_eq!( + "# Markdown Content\nThis is some **Markdown** content.\n", + &changelog.smithy_rs[0].message + ); + assert!(changelog.aws_sdk_rust.is_empty()); + } + } +}