-
-
Notifications
You must be signed in to change notification settings - Fork 238
feat(build): Add --force-git-metadata flag with CI auto-detection (EME-604) #2974
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
Conversation
|
5456ebe to
c605f87
Compare
szokeasaurusrex
left a comment
There was a problem hiding this 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)
22f1fcb to
6e13136
Compare
| /// | ||
| /// 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() |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
src/commands/build/upload.rs
Outdated
| .help("The release notes to use for the upload.") | ||
| ) | ||
| /// Holds git metadata collected for build uploads. | ||
| #[derive(Debug, Default)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/commands/build/upload.rs
Outdated
| /// | ||
| /// 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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| .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.") | ||
| ) |
There was a problem hiding this comment.
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.
tests/integration/_cases/build/build-upload-help-not-macos.trycmd
Outdated
Show resolved
Hide resolved
chromy
left a comment
There was a problem hiding this 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.
szokeasaurusrex
left a comment
There was a problem hiding this 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.
| .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.") | ||
| ) |
There was a problem hiding this comment.
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
src/commands/build/upload.rs
Outdated
| .help("The release notes to use for the upload.") | ||
| ) | ||
| /// Holds git metadata collected for build uploads. | ||
| #[derive(Debug, Default)] |
There was a problem hiding this comment.
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.
src/commands/build/upload.rs
Outdated
| /// | ||
| /// 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 { |
There was a problem hiding this comment.
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.
src/commands/build/upload.rs
Outdated
| use ini::Ini; | ||
| use std::path::PathBuf; |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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
src/commands/build/upload.rs
Outdated
| .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.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
src/commands/build/upload.rs
Outdated
| .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.") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| - 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` |
There was a problem hiding this comment.
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:
| - 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. |
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]>
3aa37ef to
3867313
Compare
…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]>
szokeasaurusrex
left a comment
There was a problem hiding this 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 🚀
Co-authored-by: Daniel Szoke <[email protected]>
Co-authored-by: Daniel Szoke <[email protected]>
Summary
--force-git-metadataand--no-git-metadataflags to control git metadata collection for build uploadsVcsInfodirectly, eliminating duplicateGitMetadatastructChanges
New CI Detection Module (
src/utils/ci.rs):Build Upload Command (
src/commands/build/upload.rs):--force-git-metadata- Force enable git metadata collection--no-git-metadata- Force disable git metadata collectionVcsInfostruct directly instead of intermediateGitMetadatastructcollect_git_metadatafunctionFixes EME-604
🤖 Generated with Claude Code