feat: ACI-4571 Get benchmark phase#232
Conversation
fcff77c to
703e446
Compare
| return fmt.Errorf("failed to create xcrun wrapper script: %w", err) | ||
| } | ||
|
|
||
| pathContent := fmt.Sprintf("export PATH=%s:$PATH", binPath) |
There was a problem hiding this comment.
Got refactored to a new package envexport
| "BITRISE_BUILD_CACHE_AUTH_TOKEN": "token", | ||
| "BITRISE_BUILD_CACHE_WORKSPACE_ID": "abc123", | ||
| "GITHUB_PATH": filepath.Join(home, ".github_path"), | ||
| "GITHUB_ENV": filepath.Join(home, ".github_env"), |
There was a problem hiding this comment.
| return TemplateInventory{}, fmt.Errorf(ErrFmtReadAuthConfig, err) | ||
| } | ||
|
|
||
| metadata := common.NewMetadata(envs, |
There was a problem hiding this comment.
This was previously in the lower methods (e.g. commonTemplateInventory), I moved it up because we need it for the benchmark phasing
| Phase string `json:"phase"` | ||
| } | ||
|
|
||
| func writeBenchmarkPhaseFile(phase string, logger log.Logger) { |
There was a problem hiding this comment.
This is a fallback in case the env var export doesn't work
| logger.Infof("(i) Benchmark phase: %s", phase) | ||
| exporter := envexport.New(envs, logger) | ||
| exporter.Export("BITRISE_BUILD_CACHE_BENCHMARK_PHASE", phase) | ||
| exporter.ExportToShellRC("Bitrise Benchmark Phase", "export BITRISE_BUILD_CACHE_BENCHMARK_PHASE="+phase) |
There was a problem hiding this comment.
This would override the user's bashrc/zshrc but we don't support benchmark phasing for local builds anyway (see https://github.com/bitrise-io/bitrise-build-cache-cli/pull/232/changes#diff-c9e446e91766670af1e9940e37c9a12bd9568e2889a3e300e9dbd8b98e038115R98)
| - content: |- | ||
| set -exo pipefail | ||
| (./gradlew compileDebugKotlin --info --stacktrace 2>&1) | tee "$BITRISE_DEPLOY_DIR/logs.txt" | ||
| (./gradlew assembleDebug --info --stacktrace 2>&1) | tee "$BITRISE_DEPLOY_DIR/logs.txt" |
There was a problem hiding this comment.
It was failing frequently because there is only 1 cacheable task
internal/config/common/benchmark.go
Outdated
| params.Set("external_workflow_name", metadata.ExternalWorkflowName) | ||
| } | ||
|
|
||
| requestURL := fmt.Sprintf("%s/build-cache/%s/invocations/gradle/command_benchmark_status?%s", |
There was a problem hiding this comment.
build tool will need to change depending on the platform (later)
There was a problem hiding this comment.
Good catch! We're doing it for Gradle only now but there's no blocker in supporting all tools in this method
SummaryAdded benchmark phase fetching from Bitrise API with support for external CI providers (CircleCI, GitHub Actions, GitLab CI). Refactored environment variable export logic into a new EnvExporter class. Enhanced cache metadata with external CI identifiers and JWT-based workspace ID extraction. Walkthrough
|
zsolt-marta-bitrise
left a comment
There was a problem hiding this comment.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
internal/config/common/benchmark.go
Outdated
| params.Set("external_workflow_name", metadata.ExternalWorkflowName) | ||
| } | ||
|
|
||
| requestURL := fmt.Sprintf("%s/build-cache/%s/invocations/gradle/command_benchmark_status?%s", |
There was a problem hiding this comment.
🐛 Bug
Missing validation for empty WorkspaceID. The function constructs a URL using c.authConfig.WorkspaceID without checking if it's empty first. This will result in a malformed URL (e.g., /build-cache//invocations/...) when WorkspaceID is empty. The test TestGetGradleBenchmarkPhase_EmptyWorkspaceID expects the function to return early with an empty string when WorkspaceID is empty, but the current implementation will attempt to make an HTTP request with an invalid URL instead.
🤖 Prompt for AI Agents:
In internal/config/common/benchmark.go at line 77, Add a check for empty WorkspaceID before constructing the URL. Return early with an empty string if WorkspaceID is empty, similar to how the function handles missing app IDs and workflow names.
zsolt-marta-bitrise
left a comment
There was a problem hiding this comment.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
| params.Set("external_workflow_name", metadata.ExternalWorkflowName) | ||
| } | ||
|
|
||
| requestURL := fmt.Sprintf("%s/build-cache/%s/invocations/%s/command_benchmark_status?%s", |
There was a problem hiding this comment.
🐛 Bug
The WorkspaceID is used to construct the URL without validation. If WorkspaceID is empty (which can happen if JWT extraction fails or returns an empty value), this creates a malformed URL like "/build-cache//invocations/...". The function should validate that WorkspaceID is not empty before constructing the request URL.
🤖 Prompt for AI Agents:
In internal/config/common/benchmark.go at line 82, Add validation to check if WorkspaceID is empty before constructing the request URL. Return an error or empty string if WorkspaceID is missing.
zsolt-marta-bitrise
left a comment
There was a problem hiding this comment.
This is an AI-generated review. Please review it carefully.
Actionable comments posted: 1
| func (c *BenchmarkPhaseClient) GetBenchmarkPhase(buildTool string, metadata CacheConfigMetadata) (string, error) { | ||
| params := url.Values{} | ||
|
|
||
| if c.authConfig.WorkspaceID == "" { |
There was a problem hiding this comment.
🐛 Bug
The function returns an error when WorkspaceID is empty, but the test TestGetBenchmarkPhase_EmptyWorkspaceID expects it to return an empty string with no error. This breaks the expected behavior where missing workspace ID should gracefully skip the benchmark phase check instead of failing.
🔄 Suggestion:
| if c.authConfig.WorkspaceID == "" { | |
| if c.authConfig.WorkspaceID == "" { | |
| c.logger.Debugf("workspace ID is not available, skipping benchmark phase check") | |
| return "", nil | |
| } |
No description provided.