Skip to content
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

feat(config): allow configuration from package/workspace metadata #1082

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
2e2446e
feat(config): allow configuration from package/workspace metadata
orhun Nov 22, 2023
0cd2b1f
chore: fix clippy lints
orhun Dec 2, 2023
b4a9e5d
Merge branch 'main' into feat/config_from_metadata
orhun Dec 2, 2023
dc09c21
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 4, 2023
2cbc802
refactor: throw an error if the config is specified both as file and …
orhun Dec 5, 2023
9ac3739
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 5, 2023
03b4e1b
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 5, 2023
77774ca
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 7, 2023
b348e2b
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 7, 2023
980c262
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 7, 2023
7f341e5
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 7, 2023
9a60435
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 11, 2023
bedc1e6
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Dec 27, 2023
e177308
remove duplicate dependency
MarcoIeni Dec 27, 2023
cbde073
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Dec 27, 2023
7be03d5
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 27, 2023
ce3a5bc
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 27, 2023
62ad070
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Dec 30, 2023
afb0392
Merge branch 'main' into feat/config_from_metadata
orhun Dec 31, 2023
3d5bf88
refactor: re-use cargo metadata
orhun Dec 31, 2023
9955593
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 1, 2024
c84ea07
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 1, 2024
604a45b
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 1, 2024
ae68908
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 1, 2024
9d8a227
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 1, 2024
263cb66
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 1, 2024
4679f2b
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Jan 1, 2024
7e349df
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Jan 1, 2024
3fa6823
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Jan 1, 2024
70e28a9
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 2, 2024
c37b11e
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Jan 3, 2024
015fcd0
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 3, 2024
1b8e0e4
Merge branch 'main' into feat/config_from_metadata
mergify[bot] Jan 6, 2024
04a26d7
test: add integration test
orhun Jan 7, 2024
9a3aa94
test: commit the changes after disabling update
orhun Jan 12, 2024
471de29
Merge branch 'main' into feat/config_from_metadata
MarcoIeni Jan 13, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion crates/release_plz/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,5 @@ assert_cmd.workspace = true
async-trait.workspace = true
fake.workspace = true
expect-test.workspace = true
serde_json.workspace = true
tempfile.workspace = true
toml_edit.workspace = true
65 changes: 57 additions & 8 deletions crates/release_plz/src/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ mod update;
use std::path::{Path, PathBuf};

use anyhow::Context;
use cargo_metadata::Metadata;
use release_plz_core::CARGO_TOML;
use tracing::info;

Expand Down Expand Up @@ -59,7 +60,11 @@ fn local_manifest(project_manifest: Option<&Path>) -> PathBuf {
}
}

fn parse_config(config_path: Option<&Path>) -> anyhow::Result<Config> {
fn parse_config(
config_path: Option<&Path>,
project_manifest: Option<&Path>,
cargo_metadata: &Metadata,
) -> anyhow::Result<Config> {
let (config, path) = if let Some(config_path) = config_path {
match std::fs::read_to_string(config_path) {
Ok(config) => (config, config_path),
Expand All @@ -71,22 +76,66 @@ fn parse_config(config_path: Option<&Path>) -> anyhow::Result<Config> {
},
}
} else {
match first_file_contents([
let file_contents = first_file_contents([
Path::new("release-plz.toml"),
Path::new(".release-plz.toml"),
])? {
Some((config, path)) => (config, path),
None => {
info!("release-plz config file not found, using default configuration");
return Ok(Config::default());
}
])?;
if let Some(project_manifest) = project_manifest
.or(Some(Path::new(CARGO_TOML)))
.filter(|v| v.exists())
Copy link
Owner

@MarcoIeni MarcoIeni Jan 13, 2024

Choose a reason for hiding this comment

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

if the project manifest exists, you ignore completely the file_contents, returning the default configuration on line 100, right? 🤔

Imagine somebody that doesn't specify the config in the Cargo.toml metadata. Their release-plz.toml file is ignored. I tested this by running your branch on release-plz itself.

{
return match parse_config_from_metadata(cargo_metadata)? {
Some(config) => {
if file_contents.is_some() {
return Err(anyhow::anyhow!(
"configuration is both specified as file and metadata"
));
}
info!(
"using configuration from metadata in {:?}",
project_manifest
);
Ok(config)
}
None => Ok(Config::default()),
};
}
if let Some((config, path)) = file_contents {
(config, path)
} else {
info!("release-plz config file not found, using default configuration");
return Ok(Config::default());
}
};

info!("using release-plz config file {}", path.display());
toml::from_str(&config).with_context(|| format!("invalid config file {config_path:?}"))
}

fn parse_config_from_metadata(metadata: &Metadata) -> anyhow::Result<Option<Config>> {
// check both workspace and package metadata
for metadata in [
Some(metadata.clone().workspace_metadata),
metadata.packages.first().map(|v| v.metadata.clone()),
Copy link
Owner

Choose a reason for hiding this comment

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

why do you only consider the first package?
Also, metadata.packages also contains your dependencies. We should only consider workspace members.
You can find workspace members with this function.

]
.into_iter()
.filter(|v| v.clone().is_some_and(|v| !v.is_null()))
{
if let Some(metadata) = metadata
// safe to unwrap() since it is checked above
.unwrap()
.as_object()
.ok_or_else(|| anyhow::anyhow!("failed to convert metadata to object"))?
.get("release-plz")
{
let config: Config = serde_json::from_value(metadata.clone())
.context("failed to parse config from metadata")?;
return Ok(Some(config));
}
}
Ok(None)
}

/// Returns the contents of the first file that exists.
///
/// If none of the files exist, returns `Ok(None)`.
Expand Down
9 changes: 7 additions & 2 deletions crates/release_plz/src/args/release.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::path::{Path, PathBuf};

use cargo_metadata::Metadata;
use clap::{
builder::{NonEmptyStringValueParser, PathBufValueParser},
ValueEnum,
Expand Down Expand Up @@ -71,8 +72,12 @@ pub enum ReleaseGitBackendKind {
}

impl Release {
pub fn config(&self) -> anyhow::Result<Config> {
super::parse_config(self.config.as_deref())
pub fn config(&self, cargo_metadata: &Metadata) -> anyhow::Result<Config> {
super::parse_config(
self.config.as_deref(),
self.optional_project_manifest(),
cargo_metadata,
)
}

pub fn release_request(
Expand Down
9 changes: 7 additions & 2 deletions crates/release_plz/src/args/update.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::path::{Path, PathBuf};

use anyhow::Context;
use cargo_metadata::Metadata;
use chrono::NaiveDate;
use clap::builder::{NonEmptyStringValueParser, PathBufValueParser};
use git_cliff_core::config::Config as GitCliffConfig;
Expand Down Expand Up @@ -101,8 +102,12 @@ impl RepoCommand for Update {
}

impl Update {
pub fn config(&self) -> anyhow::Result<Config> {
super::parse_config(self.config.as_deref())
pub fn config(&self, cargo_metadata: &Metadata) -> anyhow::Result<Config> {
super::parse_config(
self.config.as_deref(),
self.optional_project_manifest(),
Copy link
Owner

Choose a reason for hiding this comment

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

project_manifest is already inside self.config. You don't need this parameter.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

self.config is a PathBuf. Not sure if I understood.

Copy link
Owner

Choose a reason for hiding this comment

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

oops sorry. Than you can find the project manifest in cargo_metadata.workspace_root.join("Cargo.toml")

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

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

Not sure if we can do this. What happens if the user provided a custom path via --project-manifest?

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, I missed this comment.
It's already taken into account here

cargo_metadata,
)
}

fn dependencies_update(&self, config: &Config) -> bool {
Expand Down
6 changes: 3 additions & 3 deletions crates/release_plz/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,14 @@ async fn run(args: CliArgs) -> anyhow::Result<()> {
match args.command {
Command::Update(cmd_args) => {
let cargo_metadata = cmd_args.cargo_metadata()?;
let config = cmd_args.config()?;
let config = cmd_args.config(&cargo_metadata)?;
let update_request = cmd_args.update_request(config, cargo_metadata)?;
let updates = release_plz_core::update(&update_request)?;
println!("{}", updates.0.summary());
}
Command::ReleasePr(cmd_args) => {
let cargo_metadata = cmd_args.update.cargo_metadata()?;
let config = cmd_args.update.config()?;
let config = cmd_args.update.config(&cargo_metadata)?;
let pr_labels = config.workspace.pr_labels.clone();
let pr_draft = config.workspace.pr_draft;
let update_request = cmd_args.update.update_request(config, cargo_metadata)?;
Expand All @@ -51,7 +51,7 @@ async fn run(args: CliArgs) -> anyhow::Result<()> {
}
Command::Release(cmd_args) => {
let cargo_metadata = cmd_args.cargo_metadata()?;
let config = cmd_args.config()?;
let config = cmd_args.config(&cargo_metadata)?;
let request: ReleaseRequest = cmd_args.release_request(config, cargo_metadata)?;
release_plz_core::release(&request).await?;
}
Expand Down
12 changes: 12 additions & 0 deletions crates/release_plz/tests/all/changelog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,15 @@ async fn release_plz_releases_a_new_project() {

assert_eq!(packages().len(), 1);
}

#[tokio::test]
#[cfg_attr(not(feature = "docker-tests"), ignore)]
async fn release_plz_uses_config_from_manifest() {
let context = TestContext::new().await;
context.disable_changelog_update();

context.run_release_pr().success();

let opened_prs = context.opened_release_prs().await;
assert_eq!(opened_prs.len(), 0);
}
13 changes: 13 additions & 0 deletions crates/release_plz/tests/all/helpers/test_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,17 @@ impl TestContext {
.assert()
}

pub fn disable_changelog_update(&self) {
let cargo_toml_path = self.repo_dir().join("Cargo.toml");
let mut cargo_toml = LocalManifest::try_new(&cargo_toml_path).unwrap();
cargo_toml.data["workspace"]["metadata"]["release-plz"]["workspace"]["changelog_update"] =
toml_edit::Item::Value(toml_edit::Value::Boolean(toml_edit::Formatted::new(false)));

cargo_toml.write().unwrap();
let repo = Repo::new(self.repo_dir()).unwrap();
repo.add_all_and_commit("Disable changelog update").unwrap();
}
MarcoIeni marked this conversation as resolved.
Show resolved Hide resolved

pub fn repo_dir(&self) -> PathBuf {
self.test_dir.path().join(&self.gitea.repo)
}
Expand All @@ -115,6 +126,8 @@ fn commit_cargo_init(repo_dir: &Path, gitea: &GiteaContext) -> Repo {
// set email
repo.git(&["config", "user.email", "a@example.com"])
.unwrap();
// disable gpg signing
repo.git(&["config", "commit.gpgsign", "false"]).unwrap();

create_cargo_config(repo_dir, username);

Expand Down