Skip to content

Conversation

@runningcode
Copy link
Contributor

@runningcode runningcode commented Nov 20, 2025

Summary

  • Adds --force-git-metadata and --no-git-metadata flags to control git metadata collection for build uploads
  • Implements CI detection that automatically enables git metadata only when running in most CI environments (GitHub Actions, GitLab CI, Jenkins, etc.)
  • Prevents local dev builds from triggering unwanted GitHub status checks while maintaining automatic behavior for CI builds
  • Refactors git metadata handling to use VcsInfo directly, eliminating duplicate GitMetadata struct

Changes

New CI Detection Module (src/utils/ci.rs):

  • Checks common CI environment variables to determine if running in CI
  • Based on logic from sentry-android-gradle-plugin

Build Upload Command (src/commands/build/upload.rs):

  • Added two separate boolean flags for clarity:
    • --force-git-metadata - Force enable git metadata collection
    • --no-git-metadata - Force disable git metadata collection
    • Default (no flag) - Auto-detect CI environment
  • Refactored to use VcsInfo struct directly instead of intermediate GitMetadata struct
  • Simplified string handling in collect_git_metadata function

Fixes EME-604

🤖 Generated with Claude Code

@linear
Copy link

linear bot commented Nov 20, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Nov 20, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 3b07dcb

@runningcode runningcode force-pushed the no/eme-604-git-metadata-flag branch from 5456ebe to c605f87 Compare November 21, 2025 14:50
@runningcode runningcode marked this pull request as ready for review November 21, 2025 14:51
@runningcode runningcode requested review from a team and szokeasaurusrex as code owners November 21, 2025 14:51
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

I've added some mostly minor suggestions to simplify the implementation a bit.

Mainly, I think we should have just a single way to disable the Git metadata collection, unless there is a specific reason why we need two ways (--git-metadata=false and --no-git-metadata)

@runningcode runningcode force-pushed the no/eme-604-git-metadata-flag branch from 22f1fcb to 6e13136 Compare November 25, 2025 17:07
///
/// 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()
Copy link

Choose a reason for hiding this comment

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

Bug: CI detection treats CI=false as CI environment

The is_ci function checks only for the existence of the CI environment variable using is_ok, not its value. This means CI=false or CI=0 would incorrectly be detected as a CI environment. While most CI systems set CI=true, this edge case could cause unexpected behavior when developers set CI=false to test non-CI behavior locally. Consider checking if the variable equals a truthy value like "true" or "1".

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is a common thing or not. Even if it is might be better to leave this consistant with the Gradle plugin above rather than diverging the logic.

Copy link
Member

Choose a reason for hiding this comment

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

I am also unsure; this may be worth investigating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is common. The Gradle code I copied was used across many, many orgs. They an just unset this env variable to test.

.help("The release notes to use for the upload.")
)
/// Holds git metadata collected for build uploads.
#[derive(Debug, Default)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is pretty similar to the VcsInfo object but the difference is that GitMetadata uses owned String types while VcsInfo uses borrowed &str.

So we could potentially remove this but we need some other mechanism to keep the values alive. I think another option is that we could also have the collect_git_metadata return a Tuple.

What are your thoughts here?

Copy link
Member

Choose a reason for hiding this comment

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

If we want to reuse the VcsInfo struct here, I would just change the &str fields in VcsInfo to Cow<str>; that way, both borrowed and owned values can be supported. I have addressed the &u32 in #3000.

///
/// 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same as the previous implementation with the addition of auto_collect. When true, the behavior is the same as before but when set to false (when git-metadata=false) we can still use the command-line provided values (like --head-ref foo when --git-metadata=false)

Copy link
Contributor

Choose a reason for hiding this comment

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

Code is getting to be kind of deeply nested :/

One alternative approach might be the equivalent of:


def get_git_metadata_from_repo():
   return {
     "head_sha": git_repo_head_sha()
     # ...
   }
  

def get_git_metadata_from_cli():
   return {
     "head_sha": cli.head_sha or None,
     # ...
   }

def get_git_metadata():
  if should_collect_git_metadata:
    default_metadata = get_git_metadata_from_repo()
  else:
    default_metadata = {}

  manual_metadata = get_git_metadata_from_cli()

  return {
    "head_sha": manual_metadata["head_sha"] or default_metadata["head_sha"],
    # ...
  }

(Could even have: get_git_metadata_from_env for the github env inspection vs. the repo case)

This would split the priority hierarchy (cli >> env (if infer enabled) >> repo (if infer enabled) >> empty) - from the mechanics of extracting from cli vs. env vs. repo.

Alternatively maybe there is some nice rust feature for making this a bit less nested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this is a good point. @szokeasaurusrex, is there some fancy Rust feature to help with the nesting here?

Copy link
Member

Choose a reason for hiding this comment

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

The only thing I can think of is .then (will include an inline comment with an example). But, that is only a minor simplification imo.

I kinda like @chromy's suggestion of splitting the logic for collecting from the CLI from the logic for auto detection, but I'm not sure how easy/difficult it would be to implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I took a quick stab at splitting this up in to get_git_metadata_from_repo and get_git_metadata_from_cli doing this and it seems to require creating a lot of structs to hold all the different return objects for all these functions. I think it's a good idea but maybe this refactoring would make sense in a separate PR to make it easier to review?

Comment on lines 110 to 119
.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.")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused what clap is but I think that is the behavior that we want.

Copy link
Contributor

@chromy chromy left a comment

Choose a reason for hiding this comment

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

Logic seems correct to me, left some minor style comments. If Daniel is happy with the Rust then lgtm.

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Looks good to me, for the most part!

I am requesting changes because --force-git-metadata (without a value) unfortunately does not work, so we either need to change the API (my recommendation) or adjust the help text accordingly.

Comment on lines 110 to 119
.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.")
)
Copy link
Member

Choose a reason for hiding this comment

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

clap is the library which parses the command line arguments

.help("The release notes to use for the upload.")
)
/// Holds git metadata collected for build uploads.
#[derive(Debug, Default)]
Copy link
Member

Choose a reason for hiding this comment

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

If we want to reuse the VcsInfo struct here, I would just change the &str fields in VcsInfo to Cow<str>; that way, both borrowed and owned values can be supported. I have addressed the &u32 in #3000.

///
/// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

The only thing I can think of is .then (will include an inline comment with an example). But, that is only a minor simplification imo.

I kinda like @chromy's suggestion of splitting the logic for collecting from the CLI from the logic for auto detection, but I'm not sure how easy/difficult it would be to implement.

Comment on lines 867 to 864
use ini::Ini;
use std::path::PathBuf;
Copy link
Member

Choose a reason for hiding this comment

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

It's weird to have use statements inside a function body. These should instead go at the top of the mod tests block.

///
/// 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()
Copy link
Member

Choose a reason for hiding this comment

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

I am also unsure; this may be worth investigating

Comment on lines 116 to 118
.help("Controls whether to collect and send git metadata (branch, commit, etc.). \
Use --force-git-metadata to force enable, --force-git-metadata=false to force disable. \
If not specified, git metadata is automatically collected only when running in a CI environment.")
Copy link
Member

Choose a reason for hiding this comment

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

[nit] Maybe we should specify which CI environments we can autodetect? Or, just say that we can detect "most common CI environments"? To be honest, I am not sure we need to do this, though, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point but I think it is too long of a list for this help text to list all of them. I'll just say "most"

Comment on lines 116 to 118
.help("Controls whether to collect and send git metadata (branch, commit, etc.). \
Use --force-git-metadata to force enable, --force-git-metadata=false to force disable. \
If not specified, git metadata is automatically collected only when running in a CI environment.")
Copy link
Member

Choose a reason for hiding this comment

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

h: --force-git-metadata does not work; a value is required.

I feel that the requirement to supply a value makes the API clunkier. Upon further reflection, I think we should just have a --force-git-metadata flag and a --no-git-metadata flag, neither of which takes a value.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. nice catch! --force-git-metadata flag and a --no-git-metadata

CHANGELOG.md Outdated
Comment on lines 7 to 10
- Add `--force-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 `--force-git-metadata` or disable with `--force-git-metadata=false`
Copy link
Member

Choose a reason for hiding this comment

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

I would list this under "improvements" rather than features. While it is true that we have added the --force-git-metadata flag, I believe most users will rely only on the new default CI auto-detection behavior, so we should focus the changelog entry on the auto-detection as an improvement.

With the focus on the improvement in mind, I would rewrite the entry like so:

Suggested change
- Add `--force-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 `--force-git-metadata` or disable with `--force-git-metadata=false`
- For the `sentry-cli build upload` command, we now only auto-detect Git metadata when we detect we are running in a CI environment, unless the user manually overrides this behavior ([#2974](https://github.com/getsentry/sentry-cli/pull/2974)). This change prevents local development builds from triggiering GitHub status checks for size analysis.
- We can detect most common CI environments based on the environment variables these set.
- **TODO:** Once we have settled on the arguments users can use to override the behavior, we can mention those here.

runningcode and others added 14 commits December 2, 2025 14:31
This change prevents local dev builds from triggering GitHub status checks by
introducing intelligent git metadata collection control.

Changes:
- Add new CI detection module (src/utils/ci.rs) that checks common CI
  environment variables (CI, GITHUB_ACTIONS, GITLAB_CI, JENKINS_URL, etc.)
- Add --git-metadata flag to build upload command with three modes:
  * --git-metadata or --git-metadata=true: Force enable git metadata collection
  * --git-metadata=false or --no-git-metadata: Force disable git metadata collection
  * Default (no flag): Auto-detect CI environment and enable only in CI
- Refactor execute() function to conditionally collect git metadata based on flag
  and CI detection

When git metadata collection is disabled, empty values are sent for VcsInfo fields,
preventing status checks from being triggered on GitHub.

This solves two issues:
1. Local dev builds no longer trigger unwanted status checks
2. Users with unsupported VCS providers can explicitly disable git metadata

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
- Update build upload help test snapshots to include new --git-metadata and
  --no-git-metadata flags
- Add changelog entry for the new feature

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Match the original implementation exactly:
- Replace CONTINUOUS_INTEGRATION with actual CI-specific variables
- Replace BUILD_NUMBER with JENKINS_URL (already present)
- Replace CIRCLECI with CIRCLE_BUILD_URL
- Replace TRAVIS with TRAVIS_JOB_ID
- Remove BITBUCKET_BUILD_NUMBER (not in original)
- Add bamboo_resultsUrl (Bamboo)
- Add BITRISE_BUILD_URL (Bitrise)
- Add GO_SERVER_URL (GoCD)
- Add TF_BUILD (Azure Pipelines)

This provides more accurate CI detection by checking platform-specific
variables rather than generic ones.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Extract git metadata collection logic into a separate function and struct
to improve code readability. The large if/else block in execute() is now
just 5 lines instead of ~189 lines.

Also remove the --no-git-metadata flag, keeping only --git-metadata with
true/false values for consistency.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Remove the --no-git-metadata flag from the build upload help test
expectation since we're simplifying to only use --git-metadata with
boolean values.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Correct the test expectation to match the actual help output. The flag
description should say "to force disable" not just "to disable" since
--git-metadata=false explicitly overrides CI auto-detection.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
…alse (EME-604)

The --git-metadata flag now only controls automatic collection of git
metadata, not explicitly provided values. When --git-metadata=false is
specified, user-provided flags like --head-sha, --base-sha,
--vcs-provider, etc. are now respected instead of being ignored.

Previously, --git-metadata=false would use GitMetadata::default() which
completely discarded all git metadata including explicit user values.
Now, collect_git_metadata() accepts an auto_collect parameter that
controls whether automatic inference should occur while always
respecting explicitly provided values.

Added three unit tests to verify:
- Explicit --head-sha is collected even when auto_collect=false
- Auto-inference is skipped when auto_collect=false
- Explicit --vcs-provider is used when auto_collect=false

Also removed CI environment variable tests from ci.rs.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Move make_command function to appear before collect_git_metadata to
reduce the diff size of the PR and maintain conventional ordering.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Move the execute function to its original position (right after make_command)
to reduce the diff compared to master branch. This reorganization makes the
code review easier by keeping the function order consistent with the base
branch structure.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
…-604)

Move GitMetadata struct declaration to before execute function to improve
code organization, with collect_git_metadata immediately following execute
without any declarations in between.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Combine test_collect_git_metadata_respects_explicit_values_when_auto_collect_disabled
and test_collect_git_metadata_with_explicit_vcs_provider into a single test that
verifies both explicit head_sha and vcs_provider are respected when auto_collect
is disabled, while also verifying that auto-inference doesn't happen for other
fields.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
The git metadata tests were changing the working directory and writing
.sentryclirc files to set up config, which interfered with other tests
running in parallel that use relative paths to fixture files.

Simplified the tests to create minimal in-memory configs using
Config::from_file() without binding them globally or performing any
file I/O. This eliminates the need for directory changes and allows
tests to run safely in parallel.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Rename the flag to better convey that it forces git metadata collection
behavior (enable/disable) rather than just controlling git metadata.

Changes:
- Renamed --git-metadata to --force-git-metadata
- Updated CHANGELOG.md
- Updated help text snapshots

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Co-authored-by: Daniel Szoke <[email protected]>
@runningcode runningcode force-pushed the no/eme-604-git-metadata-flag branch from 3aa37ef to 3867313 Compare December 2, 2025 13:38
runningcode and others added 6 commits December 2, 2025 14:51
…ation (EME-604)

Changed VcsInfo to use Cow<'a, str> instead of &'a str to support both owned
and borrowed data. This allows collect_git_metadata to return VcsInfo directly,
eliminating the intermediate GitMetadata struct and reducing code duplication.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
… (EME-604)

Replaced the single --force-git-metadata flag with optional boolean values
with two separate boolean flags for clarity:
- --force-git-metadata: Force enable git metadata collection
- --no-git-metadata: Disable git metadata collection

When neither flag is specified, git metadata is automatically collected
when running in most CI environments.

Also moved use statements from test function bodies to module level.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Moved the --force-git-metadata and --no-git-metadata flags entry from
Features to Improvements section in the changelog.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Use the standard str::is_empty function instead of a custom helper.
Cow<str> automatically derefs to str, so str::is_empty works directly
with skip_serializing_if.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Removed redundant Cow<str> conversions by working directly with String
values throughout the function. Changed to using .cloned() instead of
.map(|s| s.to_string()) and removed redundant closures for cleaner code.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@runningcode runningcode changed the title feat(build): Add --git-metadata flag with CI auto-detection (EME-604) feat(build): Add --force-git-metadata flag with CI auto-detection (EME-604) Dec 2, 2025
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Minor suggestion to improve changelog, otherwise lgtm 🚀

runningcode and others added 2 commits December 3, 2025 14:25
Co-authored-by: Daniel Szoke <[email protected]>
Co-authored-by: Daniel Szoke <[email protected]>
@runningcode runningcode enabled auto-merge (squash) December 3, 2025 13:28
@runningcode runningcode merged commit 8505a31 into master Dec 3, 2025
29 checks passed
@runningcode runningcode deleted the no/eme-604-git-metadata-flag branch December 3, 2025 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants