-
-
Notifications
You must be signed in to change notification settings - Fork 236
feat(build): Add --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
base: master
Are you sure you want to change the base?
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)
src/commands/build/upload.rs
Outdated
| .arg( | ||
| Arg::new("no_git_metadata") | ||
| .long("no-git-metadata") | ||
| .action(ArgAction::SetTrue) | ||
| .conflicts_with("git_metadata") | ||
| .help("Disable collection of git metadata. Equivalent to --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.
To keep things simple, let's provide only a single way to disable Git metadata. I would be OK with either removing the --no-git-metadata flag, per the suggestion below, or changing the --git-metadata flag to only be a simple flag which does not take a true/false value.
| .arg( | |
| Arg::new("no_git_metadata") | |
| .long("no-git-metadata") | |
| .action(ArgAction::SetTrue) | |
| .conflicts_with("git_metadata") | |
| .help("Disable collection of git metadata. Equivalent to --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.
Good point, removed!
src/utils/ci.rs
Outdated
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::env; | ||
|
|
||
| #[test] | ||
| fn test_is_ci_with_ci_var() { | ||
| env::set_var("CI", "true"); | ||
| assert!(is_ci()); | ||
| env::remove_var("CI"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_ci_with_github_actions() { | ||
| env::set_var("GITHUB_ACTIONS", "true"); | ||
| assert!(is_ci()); | ||
| env::remove_var("GITHUB_ACTIONS"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_not_ci() { | ||
| // Clear all CI-related env vars | ||
| let ci_vars = [ | ||
| "CI", | ||
| "JENKINS_URL", | ||
| "HUDSON_URL", | ||
| "TEAMCITY_VERSION", | ||
| "CIRCLE_BUILD_URL", | ||
| "bamboo_resultsUrl", | ||
| "GITHUB_ACTIONS", | ||
| "GITLAB_CI", | ||
| "TRAVIS_JOB_ID", | ||
| "BITRISE_BUILD_URL", | ||
| "GO_SERVER_URL", | ||
| "TF_BUILD", | ||
| "BUILDKITE", | ||
| ]; | ||
|
|
||
| for var in &ci_vars { | ||
| env::remove_var(var); | ||
| } | ||
|
|
||
| assert!(!is_ci()); | ||
| } | ||
| } |
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 these tests are necessary, as the logic in this method is trivial.
If we were to keep the tests, we would need to serialize them to avoid concurrency flakes. As this complicates things further, I would prefer just not having tests here.
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| use std::env; | |
| #[test] | |
| fn test_is_ci_with_ci_var() { | |
| env::set_var("CI", "true"); | |
| assert!(is_ci()); | |
| env::remove_var("CI"); | |
| } | |
| #[test] | |
| fn test_is_ci_with_github_actions() { | |
| env::set_var("GITHUB_ACTIONS", "true"); | |
| assert!(is_ci()); | |
| env::remove_var("GITHUB_ACTIONS"); | |
| } | |
| #[test] | |
| fn test_is_not_ci() { | |
| // Clear all CI-related env vars | |
| let ci_vars = [ | |
| "CI", | |
| "JENKINS_URL", | |
| "HUDSON_URL", | |
| "TEAMCITY_VERSION", | |
| "CIRCLE_BUILD_URL", | |
| "bamboo_resultsUrl", | |
| "GITHUB_ACTIONS", | |
| "GITLAB_CI", | |
| "TRAVIS_JOB_ID", | |
| "BITRISE_BUILD_URL", | |
| "GO_SERVER_URL", | |
| "TF_BUILD", | |
| "BUILDKITE", | |
| ]; | |
| for var in &ci_vars { | |
| env::remove_var(var); | |
| } | |
| assert!(!is_ci()); | |
| } | |
| } |
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, you're right. I removed it.
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]>
22f1fcb to
6e13136
Compare
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]>
| /// | ||
| /// 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".
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]>
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?
| /// | ||
| /// 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)
src/commands/build/upload.rs
Outdated
| .arg( | ||
| Arg::new("no_git_metadata") | ||
| .long("no-git-metadata") | ||
| .action(ArgAction::SetTrue) | ||
| .conflicts_with("git_metadata") | ||
| .help("Disable collection of git metadata. Equivalent to --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.
Good point, removed!
src/utils/ci.rs
Outdated
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use std::env; | ||
|
|
||
| #[test] | ||
| fn test_is_ci_with_ci_var() { | ||
| env::set_var("CI", "true"); | ||
| assert!(is_ci()); | ||
| env::remove_var("CI"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_ci_with_github_actions() { | ||
| env::set_var("GITHUB_ACTIONS", "true"); | ||
| assert!(is_ci()); | ||
| env::remove_var("GITHUB_ACTIONS"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_is_not_ci() { | ||
| // Clear all CI-related env vars | ||
| let ci_vars = [ | ||
| "CI", | ||
| "JENKINS_URL", | ||
| "HUDSON_URL", | ||
| "TEAMCITY_VERSION", | ||
| "CIRCLE_BUILD_URL", | ||
| "bamboo_resultsUrl", | ||
| "GITHUB_ACTIONS", | ||
| "GITLAB_CI", | ||
| "TRAVIS_JOB_ID", | ||
| "BITRISE_BUILD_URL", | ||
| "GO_SERVER_URL", | ||
| "TF_BUILD", | ||
| "BUILDKITE", | ||
| ]; | ||
|
|
||
| for var in &ci_vars { | ||
| env::remove_var(var); | ||
| } | ||
|
|
||
| assert!(!is_ci()); | ||
| } | ||
| } |
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, you're right. I removed it.
| .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.
Summary
--git-metadataflag to control git metadata collection for build uploadsChanges
New CI Detection Module (
src/utils/ci.rs):Build Upload Command (
src/commands/build/upload.rs):--git-metadataflag with three modes:--git-metadataor--git-metadata=true- Force enable--git-metadata=falseor--no-git-metadata- Force disableImpact
Fixes EME-604
🤖 Generated with Claude Code