diff --git a/CHANGELOG.md b/CHANGELOG.md index 373388ef09..cc09324a50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +### Features + +- Add `--git-metadata` flag with CI auto-detection to `sentry-cli build upload` command ([#2974](https://github.com/getsentry/sentry-cli/pull/2974)) + - Git metadata is now automatically collected only when running in CI environments (GitHub Actions, GitLab CI, Jenkins, etc.) + - Local development builds no longer trigger GitHub status checks by default + - Users can force enable with `--git-metadata` or disable with `--git-metadata=false` + ### Improvements - The `sentry-cli build upload` command now automatically detects the correct branch or tag reference in non-PR GitHub Actions workflows ([#2976](https://github.com/getsentry/sentry-cli/pull/2976)). Previously, `--head-ref` was only auto-detected for pull request workflows. Now it works for push, release, and other workflow types by using the `GITHUB_REF_NAME` environment variable. diff --git a/src/commands/build/upload.rs b/src/commands/build/upload.rs index 9b605a9a86..b2df9bd435 100644 --- a/src/commands/build/upload.rs +++ b/src/commands/build/upload.rs @@ -22,6 +22,7 @@ use crate::utils::build::{ is_aab_file, is_apk_file, is_zip_file, normalize_directory, write_version_metadata, }; use crate::utils::chunks::{upload_chunks, Chunk}; +use crate::utils::ci::is_ci; use crate::utils::fs::get_sha1_checksums; use crate::utils::fs::TempDir; use crate::utils::fs::TempFile; @@ -106,6 +107,29 @@ pub fn make_command(command: Command) -> Command { .long("release-notes") .help("The release notes to use for the upload.") ) + .arg( + Arg::new("git_metadata") + .long("git-metadata") + .num_args(0..=1) + .default_missing_value("true") + .value_parser(clap::value_parser!(bool)) + .help("Controls whether to collect and send git metadata (branch, commit, etc.). \ + Use --git-metadata to force enable, --git-metadata=false to force disable. \ + If not specified, git metadata is automatically collected only when running in a CI environment.") + ) +} + +/// Holds git metadata collected for build uploads. +#[derive(Debug, Default)] +struct GitMetadata { + head_sha: Option, + vcs_provider: String, + head_repo_name: String, + head_ref: String, + base_ref: String, + base_repo_name: String, + base_sha: Option, + pr_number: Option, } pub fn execute(matches: &ArgMatches) -> Result<()> { @@ -114,169 +138,27 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { .get_many::("paths") .expect("paths argument is required"); - let head_sha = matches - .get_one::>("head_sha") - .map(|d| d.as_ref().cloned()) - .or_else(|| Some(vcs::find_head_sha().ok())) - .flatten(); - - let cached_remote = config.get_cached_vcs_remote(); - // Try to open the git repository and find the remote, but handle errors gracefully. - let (vcs_provider, head_repo_name, head_ref, base_ref, base_repo_name) = { - // Try to open the repo and get the remote URL, but don't fail if not in a repo. - let repo = git2::Repository::open_from_env().ok(); - let repo_ref = repo.as_ref(); - let remote_url = repo_ref.and_then(|repo| git_repo_remote_url(repo, &cached_remote).ok()); - - let vcs_provider = matches - .get_one("vcs_provider") - .map(String::as_str) - .map(Cow::Borrowed) - .or_else(|| { - remote_url - .as_ref() - .map(|url| get_provider_from_remote(url)) - .map(Cow::Owned) - }) - .unwrap_or_default(); - - let head_repo_name = matches - .get_one("head_repo_name") - .map(String::as_str) - .map(Cow::Borrowed) - .or_else(|| { - remote_url - .as_ref() - .map(|url| get_repo_from_remote_preserve_case(url)) - .map(Cow::Owned) - }) - .unwrap_or_default(); - - let head_ref = matches - .get_one("head_ref") - .map(String::as_str) - .map(Cow::Borrowed) - .or_else(|| { - // First try GitHub Actions environment variables - get_github_head_ref().map(Cow::Owned) - }) - .or_else(|| { - // Fallback to git repository introspection - // Note: git_repo_head_ref will return an error for detached HEAD states, - // which the error handling converts to None - this prevents sending "HEAD" as a branch name - // In that case, the user will need to provide a valid branch name. - repo_ref - .and_then(|r| match git_repo_head_ref(r) { - Ok(ref_name) => { - debug!("Found current branch reference: {ref_name}"); - Some(ref_name) - } - Err(e) => { - debug!("No valid branch reference found (likely detached HEAD): {e}"); - None - } - }) - .map(Cow::Owned) - }) - .unwrap_or_default(); - - let base_ref = matches - .get_one("base_ref") - .map(String::as_str) - .map(Cow::Borrowed) - .or_else(|| { - // First try GitHub Actions environment variables - get_github_base_ref().map(Cow::Owned) - }) - .or_else(|| { - // Fallback to git repository introspection - repo_ref - .and_then(|r| match git_repo_base_ref(r, &cached_remote) { - Ok(base_ref_name) => { - debug!("Found base reference: {base_ref_name}"); - Some(base_ref_name) - } - Err(e) => { - info!("Could not detect base branch reference: {e}"); - None - } - }) - .map(Cow::Owned) - }) - .unwrap_or_default(); - - let base_repo_name = matches - .get_one("base_repo_name") - .map(String::as_str) - .map(Cow::Borrowed) - .or_else(|| { - // Try to get the base repo name from the VCS if not provided - repo_ref - .and_then(|r| match git_repo_base_repo_name_preserve_case(r) { - Ok(Some(base_repo_name)) => { - debug!("Found base repository name: {base_repo_name}"); - Some(base_repo_name) - } - Ok(None) => { - debug!("No base repository found - not a fork"); - None - } - Err(e) => { - warn!("Could not detect base repository name: {e}"); - None - } - }) - .map(Cow::Owned) - }) - .unwrap_or_default(); - - ( - vcs_provider, - head_repo_name, - head_ref, - base_ref, - base_repo_name, - ) - }; - - // Track whether base_sha and base_ref were explicitly provided by the user - let base_sha_from_user = matches.get_one::>("base_sha").is_some(); - let base_ref_from_user = matches.get_one::("base_ref").is_some(); - - let mut base_sha = matches - .get_one::>("base_sha") - .map(|d| d.as_ref().cloned()) - .or_else(|| { - Some( - vcs::find_base_sha(&cached_remote) - .inspect_err(|e| debug!("Error finding base SHA: {e}")) - .ok() - .flatten(), - ) - }) - .flatten(); + // Determine if we should collect git metadata + let should_collect_git_metadata = + if let Some(&git_metadata) = matches.get_one::("git_metadata") { + // --git-metadata or --git-metadata=true/false was specified + git_metadata + } else { + // Default behavior: auto-detect CI + is_ci() + }; - let mut base_ref = base_ref; + debug!( + "Git metadata collection: {}", + if should_collect_git_metadata { + "enabled" + } else { + "disabled (use --git-metadata to enable)" + } + ); - // If base_sha equals head_sha and both were auto-inferred, skip setting base_sha and base_ref - // but keep head_sha (since comparing a commit to itself provides no meaningful baseline) - if !base_sha_from_user - && !base_ref_from_user - && base_sha.is_some() - && head_sha.is_some() - && base_sha == head_sha - { - debug!( - "Base SHA equals head SHA ({}), and both were auto-inferred. Skipping base_sha and base_ref, but keeping head_sha.", - base_sha.expect("base_sha is Some at this point") - ); - base_sha = None; - base_ref = "".into(); - } - let pr_number = matches - .get_one("pr_number") - .copied() - .or_else(get_github_pr_number); + // Always collect git metadata, but only perform automatic inference when enabled + let git_metadata = collect_git_metadata(matches, &config, should_collect_git_metadata); let build_configuration = matches.get_one("build_configuration").map(String::as_str); let release_notes = matches.get_one("release_notes").map(String::as_str); @@ -334,14 +216,14 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { info!("Uploading file: {}", path.display()); let bytes = ByteView::open(zip.path())?; let vcs_info = VcsInfo { - head_sha, - base_sha, - vcs_provider: &vcs_provider, - head_repo_name: &head_repo_name, - base_repo_name: &base_repo_name, - head_ref: &head_ref, - base_ref: &base_ref, - pr_number: pr_number.as_ref(), + head_sha: git_metadata.head_sha, + base_sha: git_metadata.base_sha, + vcs_provider: &git_metadata.vcs_provider, + head_repo_name: &git_metadata.head_repo_name, + base_repo_name: &git_metadata.base_repo_name, + head_ref: &git_metadata.head_ref, + base_ref: &git_metadata.base_ref, + pr_number: git_metadata.pr_number.as_ref(), }; match upload_file( &authenticated_api, @@ -400,6 +282,228 @@ pub fn execute(matches: &ArgMatches) -> Result<()> { Ok(()) } +/// Collects git metadata from arguments and VCS introspection. +/// +/// When `auto_collect` is false, only explicitly provided values are collected; +/// automatic inference from git repository and CI environment is skipped. +fn collect_git_metadata(matches: &ArgMatches, config: &Config, auto_collect: bool) -> GitMetadata { + let head_sha = matches + .get_one::>("head_sha") + .map(|d| d.as_ref().cloned()) + .or_else(|| { + if auto_collect { + Some(vcs::find_head_sha().ok()) + } else { + None + } + }) + .flatten(); + + let cached_remote = config.get_cached_vcs_remote(); + let (vcs_provider, head_repo_name, head_ref, base_ref, base_repo_name) = { + let repo = if auto_collect { + git2::Repository::open_from_env().ok() + } else { + None + }; + let repo_ref = repo.as_ref(); + let remote_url = repo_ref.and_then(|repo| git_repo_remote_url(repo, &cached_remote).ok()); + + let vcs_provider = matches + .get_one("vcs_provider") + .map(String::as_str) + .map(Cow::Borrowed) + .or_else(|| { + if auto_collect { + remote_url + .as_ref() + .map(|url| get_provider_from_remote(url)) + .map(Cow::Owned) + } else { + None + } + }) + .unwrap_or_default(); + + let head_repo_name = matches + .get_one("head_repo_name") + .map(String::as_str) + .map(Cow::Borrowed) + .or_else(|| { + if auto_collect { + remote_url + .as_ref() + .map(|url| get_repo_from_remote_preserve_case(url)) + .map(Cow::Owned) + } else { + None + } + }) + .unwrap_or_default(); + + let head_ref = matches + .get_one("head_ref") + .map(String::as_str) + .map(Cow::Borrowed) + .or_else(|| { + if auto_collect { + // First try GitHub Actions environment variables + get_github_head_ref().map(Cow::Owned) + } else { + None + } + }) + .or_else(|| { + if auto_collect { + // Fallback to git repository introspection + repo_ref + .and_then(|r| match git_repo_head_ref(r) { + Ok(ref_name) => { + debug!("Found current branch reference: {ref_name}"); + Some(ref_name) + } + Err(e) => { + debug!( + "No valid branch reference found (likely detached HEAD): {e}" + ); + None + } + }) + .map(Cow::Owned) + } else { + None + } + }) + .unwrap_or_default(); + + let base_ref = matches + .get_one("base_ref") + .map(String::as_str) + .map(Cow::Borrowed) + .or_else(|| { + if auto_collect { + // First try GitHub Actions environment variables + get_github_base_ref().map(Cow::Owned) + } else { + None + } + }) + .or_else(|| { + if auto_collect { + // Fallback to git repository introspection + repo_ref + .and_then(|r| match git_repo_base_ref(r, &cached_remote) { + Ok(base_ref_name) => { + debug!("Found base reference: {base_ref_name}"); + Some(base_ref_name) + } + Err(e) => { + info!("Could not detect base branch reference: {e}"); + None + } + }) + .map(Cow::Owned) + } else { + None + } + }) + .unwrap_or_default(); + + let base_repo_name = matches + .get_one("base_repo_name") + .map(String::as_str) + .map(Cow::Borrowed) + .or_else(|| { + if auto_collect { + // Try to get the base repo name from the VCS if not provided + repo_ref + .and_then(|r| match git_repo_base_repo_name_preserve_case(r) { + Ok(Some(base_repo_name)) => { + debug!("Found base repository name: {base_repo_name}"); + Some(base_repo_name) + } + Ok(None) => { + debug!("No base repository found - not a fork"); + None + } + Err(e) => { + warn!("Could not detect base repository name: {e}"); + None + } + }) + .map(Cow::Owned) + } else { + None + } + }) + .unwrap_or_default(); + + ( + vcs_provider, + head_repo_name, + head_ref, + base_ref, + base_repo_name, + ) + }; + + let base_sha_from_user = matches.get_one::>("base_sha").is_some(); + let base_ref_from_user = matches.get_one::("base_ref").is_some(); + + let mut base_sha = matches + .get_one::>("base_sha") + .map(|d| d.as_ref().cloned()) + .or_else(|| { + if auto_collect { + Some( + vcs::find_base_sha(&cached_remote) + .inspect_err(|e| debug!("Error finding base SHA: {e}")) + .ok() + .flatten(), + ) + } else { + None + } + }) + .flatten(); + + let mut base_ref = base_ref; + + // If base_sha equals head_sha and both were auto-inferred, skip setting base_sha and base_ref + if !base_sha_from_user + && !base_ref_from_user + && base_sha.is_some() + && head_sha.is_some() + && base_sha == head_sha + { + debug!( + "Base SHA equals head SHA ({}), and both were auto-inferred. Skipping base_sha and base_ref, but keeping head_sha.", + base_sha.expect("base_sha is Some at this point") + ); + base_sha = None; + base_ref = "".into(); + } + + let pr_number = matches.get_one("pr_number").copied().or_else(|| { + if auto_collect { + get_github_pr_number() + } else { + None + } + }); + + GitMetadata { + head_sha, + vcs_provider: vcs_provider.into_owned(), + head_repo_name: head_repo_name.into_owned(), + head_ref: head_ref.into_owned(), + base_ref: base_ref.into_owned(), + base_repo_name: base_repo_name.into_owned(), + base_sha, + pr_number, + } +} + fn handle_file(path: &Path, byteview: &ByteView) -> Result { // Handle IPA files by converting them to XCArchive #[cfg(all(target_os = "macos", target_arch = "aarch64"))] @@ -757,4 +861,109 @@ mod tests { assert!(has_symlink, "Symlink should be preserved in zip"); Ok(()) } + + #[test] + fn test_collect_git_metadata_respects_explicit_values_when_auto_collect_disabled() { + use std::env; + use tempfile::TempDir; + + // Create a temporary directory for config + let temp_dir = TempDir::new().unwrap(); + env::set_current_dir(temp_dir.path()).unwrap(); + + // Create a minimal config file + let config_path = temp_dir.path().join(".sentryclirc"); + std::fs::write(&config_path, "[defaults]\n").unwrap(); + + // Bind a minimal config + Config::from_cli_config().unwrap().bind_to_process(); + + // Create a mock ArgMatches with explicit --head-sha and --vcs-provider values + let app = make_command(Command::new("test")); + let matches = app + .try_get_matches_from(vec![ + "test", + "--org", + "test-org", + "--project", + "test-project", + "--head-sha", + "1234567890123456789012345678901234567890", + "--vcs-provider", + "custom-vcs", + "dummy.apk", + ]) + .unwrap(); + + let config = Config::current(); + + // When auto_collect is false, explicit values should still be collected + let metadata = collect_git_metadata(&matches, &config, false); + + // Explicit values should be present + assert!( + metadata.head_sha.is_some(), + "Explicit --head-sha should be collected even with auto_collect=false" + ); + assert_eq!( + metadata.vcs_provider, "custom-vcs", + "Explicit --vcs-provider should be used with auto_collect=false" + ); + + // But automatic inference should not happen for fields without explicit values + assert_eq!( + metadata.head_repo_name, "", + "head_repo_name should be empty with auto_collect=false and no explicit value" + ); + } + + #[test] + fn test_collect_git_metadata_skips_auto_inference_when_disabled() { + use std::env; + use tempfile::TempDir; + + // Create a temporary directory for config + let temp_dir = TempDir::new().unwrap(); + env::set_current_dir(temp_dir.path()).unwrap(); + + // Create a minimal config file + let config_path = temp_dir.path().join(".sentryclirc"); + std::fs::write(&config_path, "[defaults]\n").unwrap(); + + // Bind a minimal config + Config::from_cli_config().unwrap().bind_to_process(); + + // Create a mock ArgMatches without any explicit git metadata values + let app = make_command(Command::new("test")); + let matches = app + .try_get_matches_from(vec![ + "test", + "--org", + "test-org", + "--project", + "test-project", + "dummy.apk", + ]) + .unwrap(); + + let config = Config::current(); + + // When auto_collect is false and no explicit values provided, + // we should get empty metadata + let metadata = collect_git_metadata(&matches, &config, false); + + // With auto_collect=false, we should get empty values + assert_eq!( + metadata.vcs_provider, "", + "vcs_provider should be empty with auto_collect=false and no explicit value" + ); + assert_eq!( + metadata.head_repo_name, "", + "head_repo_name should be empty with auto_collect=false and no explicit value" + ); + assert_eq!( + metadata.head_ref, "", + "head_ref should be empty with auto_collect=false and no explicit value" + ); + } } diff --git a/src/utils/ci.rs b/src/utils/ci.rs new file mode 100644 index 0000000000..c7848795cc --- /dev/null +++ b/src/utils/ci.rs @@ -0,0 +1,33 @@ +use std::env; + +/// Detects if the current environment is a CI environment by checking common CI environment variables. +/// +/// This checks environment variables that are commonly set by CI providers like: +/// - GitHub Actions +/// - GitLab CI +/// - Jenkins / Hudson +/// - CircleCI +/// - Travis CI +/// - TeamCity +/// - Bamboo +/// - Bitrise +/// - GoCD +/// - Azure Pipelines +/// - Buildkite +/// +/// Based on: https://github.com/getsentry/sentry-android-gradle-plugin/blob/15068f51fee344c00efdbec5a172297be4719b86/plugin-build/src/main/kotlin/io/sentry/android/gradle/util/CI.kt#L9 +pub fn is_ci() -> bool { + env::var("CI").is_ok() + || env::var("JENKINS_URL").is_ok() + || env::var("HUDSON_URL").is_ok() + || env::var("TEAMCITY_VERSION").is_ok() + || env::var("CIRCLE_BUILD_URL").is_ok() + || env::var("bamboo_resultsUrl").is_ok() + || env::var("GITHUB_ACTIONS").is_ok() + || env::var("GITLAB_CI").is_ok() + || env::var("TRAVIS_JOB_ID").is_ok() + || env::var("BITRISE_BUILD_URL").is_ok() + || env::var("GO_SERVER_URL").is_ok() + || env::var("TF_BUILD").is_ok() + || env::var("BUILDKITE").is_ok() +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 54bdc992e1..d4f75b7241 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -5,6 +5,7 @@ pub mod args; pub mod auth_token; pub mod build; pub mod chunks; +pub mod ci; pub mod cordova; pub mod dif; pub mod dif_upload; diff --git a/tests/integration/_cases/build/build-upload-help-macos.trycmd b/tests/integration/_cases/build/build-upload-help-macos.trycmd index 0411e4dc2a..80cd069b96 100644 --- a/tests/integration/_cases/build/build-upload-help-macos.trycmd +++ b/tests/integration/_cases/build/build-upload-help-macos.trycmd @@ -53,6 +53,11 @@ Options: be used. --release-notes The release notes to use for the upload. + --git-metadata [] + Controls whether to collect and send git metadata (branch, commit, etc.). Use + --git-metadata to force enable, --git-metadata=false to force disable. If not specified, + git metadata is automatically collected only when running in a CI environment. [possible + values: true, false] -h, --help Print help diff --git a/tests/integration/_cases/build/build-upload-help-not-macos.trycmd b/tests/integration/_cases/build/build-upload-help-not-macos.trycmd index 0415496ac0..309d64ee5e 100644 --- a/tests/integration/_cases/build/build-upload-help-not-macos.trycmd +++ b/tests/integration/_cases/build/build-upload-help-not-macos.trycmd @@ -52,6 +52,11 @@ Options: be used. --release-notes The release notes to use for the upload. + --git-metadata [] + Controls whether to collect and send git metadata (branch, commit, etc.). Use + --git-metadata to force enable, --git-metadata=false to force disable. If not specified, + git metadata is automatically collected only when running in a CI environment. [possible + values: true, false] -h, --help Print help