WIP: Allow ART to build with code coverage#2450
WIP: Allow ART to build with code coverage#2450jupierce wants to merge 6 commits intoopenshift-eng:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWhen build_profiles.enable_go_cover is true, build metadata/envs receive GO_COMPLIANCE_COVER=1, util.inject_coverage_server is invoked to copy a Go coverage_server.go into discovered main packages, and ART Go wrapper scripts add coverage flags and may append the coverage server source during go build/install. New JSON Schema definitions support enabling this feature via build profiles. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as GroupConfig.build_profiles
participant Distgit as Distgit/Rebaser
participant Util as util.inject_coverage_server
participant Docker as Dockerfile/BuildContext
participant Wrapper as ART Go wrapper
participant GoBuild as go build/install
participant Coverage as coverage_server.go
Config->>Distgit: enable_go_cover = true
Distgit->>Docker: add ENV GO_COMPLIANCE_COVER=1
Distgit->>Util: inject_coverage_server(dest_dir)
Util-->>Distgit: copies coverage_server.go into main package dirs
Docker->>Wrapper: run build/install with GO_COMPLIANCE_COVER=1
Wrapper->>GoBuild: append -cover -covermode=atomic (and server file if found)
GoBuild->>Coverage: runtime serves /coverage endpoint
Coverage-->>GoBuild: returns coverage payload (meta + counters)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
jupierce
force-pushed
the
coverage_stream
branch
from
d001dcb to
86cda79
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/resources/coverage_server.go`:
- Around line 103-108: The handler currently calls
json.NewEncoder(w).Encode(response) which streams directly to the ResponseWriter
so a partial write can leave headers sent and http.Error ineffective; change
this to encode into a temporary buffer (e.g., use json.Marshal or
json.NewEncoder(&bytes.Buffer)) and check for encoding errors first, then set
w.Header().Set("Content-Type","application/json"), write any status code, and
finally write the buffer to w while checking the write error; update the block
around json.NewEncoder(w).Encode, http.Error, and the response variable to
perform buffer encoding and guarded writing instead.
🧹 Nitpick comments (1)
doozer/doozerlib/resources/coverage_server.go (1)
77-84: Hash extraction relies on internal metadata format.The hardcoded byte offsets
metaData[16:32]assume a specific binary layout of Go's coverage metadata. This could produce incorrect hashes if the format changes in future Go versions. The fallback to"unknown"prevents crashes but may lead to filename collisions.Consider adding a comment documenting the assumed format and the Go version it was tested against.
| w.Header().Set("Content-Type", "application/json") | ||
| if err := json.NewEncoder(w).Encode(response); err != nil { | ||
| log.Printf("[COVERAGE] Error encoding response: %v", err) | ||
| http.Error(w, "Failed to encode response", http.StatusInternalServerError) | ||
| return | ||
| } |
There was a problem hiding this comment.
Error response after partial write may fail silently.
json.NewEncoder(w).Encode() writes directly to the ResponseWriter. If encoding fails after headers are sent or data is partially written, the subsequent http.Error() won't take effect because HTTP headers can only be written once.
To handle this correctly, encode to a buffer first, then write to the response.
🔧 Proposed fix
- w.Header().Set("Content-Type", "application/json")
- if err := json.NewEncoder(w).Encode(response); err != nil {
- log.Printf("[COVERAGE] Error encoding response: %v", err)
- http.Error(w, "Failed to encode response", http.StatusInternalServerError)
- return
- }
+ jsonData, err := json.Marshal(response)
+ if err != nil {
+ log.Printf("[COVERAGE] Error encoding response: %v", err)
+ http.Error(w, "Failed to encode response", http.StatusInternalServerError)
+ return
+ }
+
+ w.Header().Set("Content-Type", "application/json")
+ if _, err := w.Write(jsonData); err != nil {
+ log.Printf("[COVERAGE] Error writing response: %v", err)
+ return
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| w.Header().Set("Content-Type", "application/json") | |
| if err := json.NewEncoder(w).Encode(response); err != nil { | |
| log.Printf("[COVERAGE] Error encoding response: %v", err) | |
| http.Error(w, "Failed to encode response", http.StatusInternalServerError) | |
| return | |
| } | |
| jsonData, err := json.Marshal(response) | |
| if err != nil { | |
| log.Printf("[COVERAGE] Error encoding response: %v", err) | |
| http.Error(w, "Failed to encode response", http.StatusInternalServerError) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "application/json") | |
| if _, err := w.Write(jsonData); err != nil { | |
| log.Printf("[COVERAGE] Error writing response: %v", err) | |
| return | |
| } |
🤖 Prompt for AI Agents
In `@doozer/doozerlib/resources/coverage_server.go` around lines 103 - 108, The
handler currently calls json.NewEncoder(w).Encode(response) which streams
directly to the ResponseWriter so a partial write can leave headers sent and
http.Error ineffective; change this to encode into a temporary buffer (e.g., use
json.Marshal or json.NewEncoder(&bytes.Buffer)) and check for encoding errors
first, then set w.Header().Set("Content-Type","application/json"), write any
status code, and finally write the buffer to w while checking the write error;
update the block around json.NewEncoder(w).Encode, http.Error, and the response
variable to perform buffer encoding and guarded writing instead.
78efc5e to
5ed2ddd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/resources/coverage_server.go`:
- Around line 13-14: coverage_server.go imports runtime/coverage (requires Go
1.20+) but the repo has no go.mod and the Dockerfile/build do not compile Go, so
the file cannot build; either (A) add Go module and build integration: create a
go.mod with "go 1.20", add the package to the module, update the
Dockerfile/build pipeline to install Go and run go build for the package (or add
a specific build target invoked by CI), or (B) remove/disable the file if
unused: delete coverage_server.go (and its runtime/coverage import) or add a
build tag (e.g., //go:build coverage) so it is excluded unless explicitly built;
reference the file name coverage_server.go and the runtime/coverage import when
making the change.
In `@doozer/doozerlib/rpm_builder.py`:
- Around line 418-422: The generated shim written to $GOSHIM_DIR/go uses an
invalid shebang '#/bin/sh' which causes exec format errors; update the shim
template in rpm_builder.py where the heredoc is created (the block feeding cat >
$GOSHIM_DIR/go) to use a valid shebang '#!/bin/sh' (fix the string that
currently contains '#/bin/sh') while keeping the rest of the heredoc content
(GOSHIM_DIR, GO_COMPLIANCE_COVER logic, and rpm_builder_go_wrapper_sh)
unchanged.
- Around line 122-125: The coverage-server injection currently runs when
rpm.runtime.group_config.build_profiles.enable_go_cover is true
(util.inject_coverage_server), but the build flag GO_COMPLIANCE_COVER is only
emitted when go_compliance_shim is enabled in _populate_specfile_async; update
the logic so injection is gated to the actual shim flag (i.e., only call
util.inject_coverage_server when go_compliance_shim is enabled too) or
alternatively ensure GO_COMPLIANCE_COVER is set for non-shim builds; modify the
conditional around inject_coverage_server to check
rpm.runtime.group_config.build_profiles.go_compliance_shim (or add an explicit
runtime assertion/fail-fast error when enable_go_cover is true but
go_compliance_shim is false) so the source is only injected when the build will
compile with -cover.
In `@doozer/doozerlib/util.py`:
- Around line 1016-1019: The loop that injects _COVERAGE_SERVER_GO into
main_dirs uses shutil.copy2 which will overwrite any existing
coverage_server.go; update the logic inside the loop (where main_dirs, dest and
_COVERAGE_SERVER_GO are used and shutil.copy2 is called) to first check if
dest.exists() and if so either skip the copy or only overwrite when contents
differ (compare with filecmp.cmp or byte-read comparison), and only call
shutil.copy2 when safe; adjust logger_instance.info to reflect whether the file
was injected, skipped, or unchanged.
🧹 Nitpick comments (1)
doozer/doozerlib/util.py (1)
961-975: Avoid false negatives whenpackage mainappears after long headers.Limiting the scan to 50 lines can miss
package mainin generated files or long license headers, which would silently skip injection. Consider scanning until the firstpackagedeclaration (or using Go parsing).Possible improvement
- try: - with file_path.open('rb') as f: - for _ in range(50): - line = f.readline() - if not line: - break - if _PKG_MAIN_PATTERN.match(line): - return True + try: + with file_path.open('rb') as f: + for line in f: + if _PKG_MAIN_PATTERN.match(line): + return True + if line.lstrip().startswith(b'package '): + return False
| "runtime/coverage" | ||
| "time" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the go.mod file for minimum Go version
if [ -f "go.mod" ]; then
head -20 go.mod | grep -E "^go "
fiRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check coverage_server.go for build tags and full context
cat -n doozer/doozerlib/resources/coverage_server.go | head -30Repository: openshift-eng/art-tools
Length of output: 937
🏁 Script executed:
# Search for any Go version-related constraints or build tags in the codebase
rg "build go1\.|//go:build|// +build" --type go -A 2 -B 2 | head -50Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Find go.mod file
fd "go\.mod" --type fRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check if there's a go.mod at the repository root and show its first 15 lines
if [ -f "./go.mod" ]; then
cat -n ./go.mod | head -15
else
find . -name "go.mod" -type f -exec sh -c 'echo "Found: {}"; head -15 "$1"' _ {} \;
fiRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check for any coverage-related files or alternative implementations
fd "coverage" --type f | grep -E "\.(go|mod)$"Repository: openshift-eng/art-tools
Length of output: 112
🌐 Web query:
When was runtime/coverage package introduced in Go - Go version requirement
💡 Result:
The standard library package runtime/coverage was introduced in Go 1.20 (released February 1, 2023). Therefore, the minimum Go version requirement is Go 1.20+. [1][2]
🏁 Script executed:
# Check the repository root structure
ls -la | head -20Repository: openshift-eng/art-tools
Length of output: 1238
🏁 Script executed:
# Look for all go.mod files in the entire repo (not just root)
find . -name "go.mod" -type f 2>/dev/nullRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check if there's a go.work file (workspace)
find . -name "go.work" -type f 2>/dev/nullRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Look at the parent directory structure of coverage_server.go
ls -la doozer/doozerlib/resources/ | head -20Repository: openshift-eng/art-tools
Length of output: 245
🏁 Script executed:
# Check doozer directory structure
ls -la doozer/ | head -20Repository: openshift-eng/art-tools
Length of output: 1226
🏁 Script executed:
# Look deeper in doozer for go.mod
find doozer -name "go.mod" -o -name "go.work" 2>/dev/nullRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check if coverage_server.go is actually being compiled or used
rg "coverage_server" --type go doozer/Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Look for Python build configuration or setup files
find doozer -name "setup.py" -o -name "pyproject.toml" -o -name "Makefile" 2>/dev/null | head -10Repository: openshift-eng/art-tools
Length of output: 92
🏁 Script executed:
# Check the build_doozer script for compilation instructions
cat doozer/build_doozerRepository: openshift-eng/art-tools
Length of output: 424
🏁 Script executed:
# Check the Dockerfile for Go compilation setup
cat doozer/Dockerfile | grep -A 5 -B 5 -i "go\|coverage"Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check setup.py to see if it references coverage_server or Go compilation
cat doozer/rundoozer/setup.pyRepository: openshift-eng/art-tools
Length of output: 765
🏁 Script executed:
# Check if coverage_server.go is referenced in any config files
rg "coverage_server" --type toml --type yaml --type json --type txtRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Double-check by looking for any references to coverage or the file
rg -i "coverage" doozer/doozerlib/resources/ --type goRepository: openshift-eng/art-tools
Length of output: 2887
🏁 Script executed:
# Check git history to see when this file was added
cd doozer/doozerlib/resources && git log --oneline coverage_server.go 2>/dev/null | head -5Repository: openshift-eng/art-tools
Length of output: 98
Add Go module configuration and build integration or remove unused code.
coverage_server.go imports runtime/coverage, which requires Go 1.20+ and has no build guards. However, the fundamental issue is that there is no go.mod file in the repository, and the file is not integrated into any build system (the Dockerfile contains no Go compiler setup). The file was recently added but is not referenced anywhere in the codebase and cannot currently compile. Either:
- Add proper Go module setup (
go.mod) and integrate compilation into the Dockerfile and build pipeline, or - Remove the file and the import if it is not intended for immediate use.
🤖 Prompt for AI Agents
In `@doozer/doozerlib/resources/coverage_server.go` around lines 13 - 14,
coverage_server.go imports runtime/coverage (requires Go 1.20+) but the repo has
no go.mod and the Dockerfile/build do not compile Go, so the file cannot build;
either (A) add Go module and build integration: create a go.mod with "go 1.20",
add the package to the module, update the Dockerfile/build pipeline to install
Go and run go build for the package (or add a specific build target invoked by
CI), or (B) remove/disable the file if unused: delete coverage_server.go (and
its runtime/coverage import) or add a build tag (e.g., //go:build coverage) so
it is excluded unless explicitly built; reference the file name
coverage_server.go and the runtime/coverage import when making the change.
| if rpm.runtime.group_config.build_profiles.enable_go_cover is True: | ||
| # Inject the coverage HTTP server source into every Go main package | ||
| # directory so that it is compiled into the binary via its init() function. | ||
| util.inject_coverage_server(Path(rpm.source_path), logger) |
There was a problem hiding this comment.
Align coverage injection with the shim that actually enables -cover.
inject_coverage_server runs whenever enable_go_cover is true, but GO_COMPLIANCE_COVER is only emitted when go_compliance_shim is enabled in _populate_specfile_async. If these flags can diverge, the server is injected but the build won’t compile with coverage, so /coverage will return errors. Consider failing fast or gating injection on go_compliance_shim (or otherwise ensure GO_COMPLIANCE_COVER is set for non‑shim builds).
Proposed guard to prevent a mismatched configuration
- if rpm.runtime.group_config.build_profiles.enable_go_cover is True:
+ if rpm.runtime.group_config.build_profiles.enable_go_cover is True:
+ if not go_compliance_shim:
+ raise ValueError(
+ "enable_go_cover requires compliance.rpm_shim.enabled to emit -cover flags"
+ )
# Inject the coverage HTTP server source into every Go main package
# directory so that it is compiled into the binary via its init() function.
util.inject_coverage_server(Path(rpm.source_path), logger)🤖 Prompt for AI Agents
In `@doozer/doozerlib/rpm_builder.py` around lines 122 - 125, The coverage-server
injection currently runs when
rpm.runtime.group_config.build_profiles.enable_go_cover is true
(util.inject_coverage_server), but the build flag GO_COMPLIANCE_COVER is only
emitted when go_compliance_shim is enabled in _populate_specfile_async; update
the logic so injection is gated to the actual shim flag (i.e., only call
util.inject_coverage_server when go_compliance_shim is enabled too) or
alternatively ensure GO_COMPLIANCE_COVER is set for non-shim builds; modify the
conditional around inject_coverage_server to check
rpm.runtime.group_config.build_profiles.go_compliance_shim (or add an explicit
runtime assertion/fail-fast error when enable_go_cover is true but
go_compliance_shim is false) so the source is only injected when the build will
compile with -cover.
| # Use single quotes 'EOF' to avoid variable expansion within script content. | ||
| cat > $GOSHIM_DIR/go << 'EOF' | ||
| #!/bin/sh | ||
| export GO_COMPLIANCE_COVER={'1' if rpm.runtime.group_config.build_profiles.enable_go_cover is True else '0'} | ||
| {rpm_builder_go_wrapper_sh} |
There was a problem hiding this comment.
Fix the invalid shebang in the generated shim.
#/bin/sh is missing the !, so the wrapper will fail with an “exec format error” when invoked via PATH.
🐛 Suggested fix
-#/bin/sh
+#!/bin/sh🤖 Prompt for AI Agents
In `@doozer/doozerlib/rpm_builder.py` around lines 418 - 422, The generated shim
written to $GOSHIM_DIR/go uses an invalid shebang '#/bin/sh' which causes exec
format errors; update the shim template in rpm_builder.py where the heredoc is
created (the block feeding cat > $GOSHIM_DIR/go) to use a valid shebang
'#!/bin/sh' (fix the string that currently contains '#/bin/sh') while keeping
the rest of the heredoc content (GOSHIM_DIR, GO_COMPLIANCE_COVER logic, and
rpm_builder_go_wrapper_sh) unchanged.
| for pkg_dir in main_dirs: | ||
| dest = pkg_dir / _COVERAGE_SERVER_GO.name | ||
| shutil.copy2(str(_COVERAGE_SERVER_GO), str(dest)) | ||
| logger_instance.info('Injected %s into %s', _COVERAGE_SERVER_GO.name, pkg_dir) |
There was a problem hiding this comment.
Avoid clobbering an existing coverage_server.go.
copy2 will overwrite any existing file with the same name. If a repo already contains a file named coverage_server.go, this will silently replace it. Consider skipping injection when the destination exists (or only overwriting when the contents match).
Safer injection
for pkg_dir in main_dirs:
dest = pkg_dir / _COVERAGE_SERVER_GO.name
- shutil.copy2(str(_COVERAGE_SERVER_GO), str(dest))
+ if dest.exists():
+ logger_instance.debug('%s already present in %s; skipping injection', _COVERAGE_SERVER_GO.name, pkg_dir)
+ continue
+ shutil.copy2(str(_COVERAGE_SERVER_GO), str(dest))
logger_instance.info('Injected %s into %s', _COVERAGE_SERVER_GO.name, pkg_dir)🤖 Prompt for AI Agents
In `@doozer/doozerlib/util.py` around lines 1016 - 1019, The loop that injects
_COVERAGE_SERVER_GO into main_dirs uses shutil.copy2 which will overwrite any
existing coverage_server.go; update the logic inside the loop (where main_dirs,
dest and _COVERAGE_SERVER_GO are used and shutil.copy2 is called) to first check
if dest.exists() and if so either skip the copy or only overwrite when contents
differ (compare with filecmp.cmp or byte-read comparison), and only call
shutil.copy2 when safe; adjust logger_instance.info to reflect whether the file
was injected, skipped, or unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/golang_builder_FIPS_wrapper.sh`:
- Around line 327-342: Pre-scan the incoming build arguments before the existing
injection block to detect if "coverage_server.go" is already present (check the
basename of each element in the args list/array) and, if found, set
COVERAGE_ADDED=1 so the injection logic (which checks GO_COMPLIANCE_COVER,
IN_BUILD, COVERAGE_ADDED, and arg) will skip adding a duplicate; update the code
that manipulates ARGS and the injection condition to rely on this flag
(referencing COVERAGE_ADDED, ARGS, and the existing coverage_server.go checks)
so you never append coverage_server.go twice.
2ebafe8 to
7bf803a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/util.py`:
- Around line 844-848: The golang-priority branch currently returns
constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2 which can exceed the allowed 1–10
Konflux range; update the 'golang' branch (the block that checks if 'golang' in
group and currently returns str(constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2))
to clamp the computed priority to a maximum of 10 (e.g., compute priority =
min(constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2, 10)) and then return it as a
string so the value always stays within the valid Konflux range.
doozer/doozerlib/util.py
Outdated
| # 3. Golang builder images are high priority since they block other builds. | ||
| if 'golang' in group: | ||
| # Prioritize since golang builds may be part of addressing security issues. | ||
| logger.info(f"Using golang priority for {metadata.distgit_key}") | ||
| return str(constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2) |
There was a problem hiding this comment.
Clamp Golang priority to the valid Konflux range.
The function contract describes a 1–10 priority; adding 2 can exceed the upper bound if the default is already high. Consider clamping to 10 to avoid invalid priorities.
🔧 Proposed fix
- return str(constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2)
+ return str(min(10, constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2))🤖 Prompt for AI Agents
In `@doozer/doozerlib/util.py` around lines 844 - 848, The golang-priority branch
currently returns constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2 which can exceed
the allowed 1–10 Konflux range; update the 'golang' branch (the block that
checks if 'golang' in group and currently returns
str(constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2)) to clamp the computed
priority to a maximum of 10 (e.g., compute priority =
min(constants.KONFLUX_DEFAULT_BUILD_PRIORITY + 2, 10)) and then return it as a
string so the value always stays within the valid Konflux range.
7bf803a to
ffdee51
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/rpm_builder_go_wrapper.sh`:
- Around line 70-77: When GO_COMPLIANCE_COVER is set the script unconditionally
adds -cover flags (lines manipulating ARGS and echoerr), which only work on Go
1.20+. Add a runtime Go version check: when GO_COMPLIANCE_COVER==1 call `go
version`, parse the major.minor, and if it’s lower than 1.20 log an error via
echoerr and exit non-zero (or disable coverage with a clear message), before
appending "-cover" and "-covermode=atomic" to ARGS; reference the
GO_COMPLIANCE_COVER variable, the ARGS array, and the echoerr/IN_BUILD logic to
place the check.
| IN_BUILD="1" # This is a go build invocation | ||
| ARGS+=("${arg}") # Add "build" | ||
|
|
||
| if [[ "${GO_COMPLIANCE_COVER:-}" == "1" ]]; then | ||
| echoerr "adding -cover for build or install operation" | ||
| ARGS+=("-cover") | ||
| ARGS+=("-covermode=atomic") | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Go version constraints in the repository
find . -maxdepth 3 -type f \( -name "go.mod" -o -name "Dockerfile" -o -name ".github" -o -name ".gitlab-ci.yml" -o -name "Makefile" \) | head -20Repository: openshift-eng/art-tools
Length of output: 177
🏁 Script executed:
# Look for go.mod file
fd "go.mod" -t fRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check for CI/CD configurations that specify Go version
fd -e yml -e yaml | xargs grep -l "go" | head -10Repository: openshift-eng/art-tools
Length of output: 195
🌐 Web query:
go build -cover flag support when was it introduced Go release history
💡 Result:
go build -cover was introduced in Go 1.20 (released February 2023) as part of the new support for collecting coverage profiles from built programs/integration tests (by running the instrumented binary with GOCOVERDIR set). [1] [2] [3]
(For context: coverage via go test -cover dates back to Go 1.2.) [2] [3]
Sources:
[1] Go 1.20 release notes (go build now supports -cover)
[2] Go blog: Code coverage for Go integration tests (dated 2023-03-08; “available in Go 1.20”)
[3] Go documentation: Coverage profiling support for integration tests
🏁 Script executed:
# Check doozer Dockerfile for Go version
cat doozer/Dockerfile | head -50Repository: openshift-eng/art-tools
Length of output: 2126
🏁 Script executed:
# Check Makefile for Go version
cat MakefileRepository: openshift-eng/art-tools
Length of output: 1496
🏁 Script executed:
# Look for go.mod anywhere in the repo
find . -name "go.mod" -type fRepository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check if there's a .go-version or similar file
fd -e "txt" | xargs grep -l "1\.[0-9]" 2>/dev/null | grep -i go | head -5Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Read the rpm_builder_go_wrapper.sh script to understand its context
cat -n doozer/doozerlib/rpm_builder_go_wrapper.shRepository: openshift-eng/art-tools
Length of output: 8641
🏁 Script executed:
# Look for CI/CD pipeline files that might specify Go version
fd -e yml -e yaml | xargs grep -A 5 -B 5 "go" 2>/dev/null | grep -i version | head -20Repository: openshift-eng/art-tools
Length of output: 49
🏁 Script executed:
# Check for README or documentation files that mention Go requirements
find . -maxdepth 2 -type f \( -name "README*" -o -name "*.md" \) | xargs grep -i "go\|version" 2>/dev/null | head -30Repository: openshift-eng/art-tools
Length of output: 4509
Require Go 1.20+ or add a version check for -cover flag support.
The -cover flag for go build was introduced in Go 1.20 (February 2023). Line 73–77 adds this flag when GO_COMPLIANCE_COVER=1, but there's no enforcement of a minimum Go version. If the environment running this wrapper uses Go < 1.20 with GO_COMPLIANCE_COVER=1 enabled, the build will fail. Either document that Go 1.20+ is required for this compliance coverage feature, or add a runtime version check to prevent silent failures.
🤖 Prompt for AI Agents
In `@doozer/doozerlib/rpm_builder_go_wrapper.sh` around lines 70 - 77, When
GO_COMPLIANCE_COVER is set the script unconditionally adds -cover flags (lines
manipulating ARGS and echoerr), which only work on Go 1.20+. Add a runtime Go
version check: when GO_COMPLIANCE_COVER==1 call `go version`, parse the
major.minor, and if it’s lower than 1.20 log an error via echoerr and exit
non-zero (or disable coverage with a clear message), before appending "-cover"
and "-covermode=atomic" to ARGS; reference the GO_COMPLIANCE_COVER variable, the
ARGS array, and the echoerr/IN_BUILD logic to place the check.
Go can target a directory or specific files. Inject coverage_server to specific files list.
jupierce
force-pushed
the
coverage_stream
branch
from
ffdee51 to
491ed36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/doozerlib/util.py`:
- Around line 832-835: The helper higher_by uses the variable phase in its
logger call but phase is assigned later, causing a NameError; either move the
phase definition to before the higher_by function or remove/replace phase from
the log message so higher_by no longer references an undefined variable. Update
the code paths that call higher_by (notably the golang builder branch) to ensure
phase is defined first if you choose to keep phase in logs, or change the logger
in higher_by to log without phase (e.g., only metadata.distgit_key and
new_priority) so it is safe regardless of call order.
🧹 Nitpick comments (1)
doozer/doozerlib/golang_builder_FIPS_wrapper.sh (1)
1-1: Shebang declares POSIXshbut script uses bash-isms.The script uses
[[ ]], arrays (+=), and==comparisons which are bash-specific. While this is a pre-existing pattern in the file, consider updating the shebang to#!/bin/bashfor correctness if the script depends on bash features.
| def higher_by(val: int) -> str: | ||
| new_priority = str(max(1, constants.KONFLUX_DEFAULT_BUILD_PRIORITY - val)) | ||
| logger.info(f"Using phase-based priority for {metadata.distgit_key}: {new_priority} (phase: {phase})") | ||
| return new_priority |
There was a problem hiding this comment.
NameError: phase is referenced before assignment.
The higher_by helper references phase in the log message (line 834), but when called from line 852 (golang builder branch), phase hasn't been defined yet—it's only assigned at line 855. This will cause a runtime exception.
🐛 Proposed fix: move phase definition earlier or adjust log message
def higher_by(val: int) -> str:
new_priority = str(max(1, constants.KONFLUX_DEFAULT_BUILD_PRIORITY - val))
- logger.info(f"Using phase-based priority for {metadata.distgit_key}: {new_priority} (phase: {phase})")
+ logger.info(f"Using computed priority for {metadata.distgit_key}: {new_priority}")
return new_priorityAlternatively, move the phase assignment before the higher_by definition if you want to retain phase in the log.
🤖 Prompt for AI Agents
In `@doozer/doozerlib/util.py` around lines 832 - 835, The helper higher_by uses
the variable phase in its logger call but phase is assigned later, causing a
NameError; either move the phase definition to before the higher_by function or
remove/replace phase from the log message so higher_by no longer references an
undefined variable. Update the code paths that call higher_by (notably the
golang builder branch) to ensure phase is defined first if you choose to keep
phase in logs, or change the logger in higher_by to log without phase (e.g.,
only metadata.distgit_key and new_priority) so it is safe regardless of call
order.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@doozer/tests/test_util.py`:
- Around line 1-3: Run the code formatter to apply Ruff's recommended formatting
to the test module test_util: execute ruff format on that file to reorder/format
the imports (logging, pathlib, tempfile), apply any whitespace/line-ending
changes Ruff suggests, save the file, and commit the formatted changes so CI
stops failing.
| import logging | ||
| import pathlib | ||
| import tempfile |
There was a problem hiding this comment.
Fix Ruff formatting to unblock CI.
The unit-tests job reports ruff format would reformat this file. Run ruff format doozer/tests/test_util.py (or ruff format) and commit the formatting changes.
🧰 Tools
🪛 GitHub Actions: unit-tests
[error] 1-1: Ruff format check failed. 1 file would be reformatted: 'doozer/tests/test_util.py'. Run 'ruff format' to fix code style issues.
🤖 Prompt for AI Agents
In `@doozer/tests/test_util.py` around lines 1 - 3, Run the code formatter to
apply Ruff's recommended formatting to the test module test_util: execute ruff
format on that file to reorder/format the imports (logging, pathlib, tempfile),
apply any whitespace/line-ending changes Ruff suggests, save the file, and
commit the formatted changes so CI stops failing.
Found issues building golang-github-prometheus-prometheus where a file with package main existed in a .go file, but it was a utility cmd with //build:ignore. Ensure we spot these a skip the directory.
jupierce
force-pushed
the
coverage_stream
branch
from
e96014a to
2a78eac
Compare
This can lead to coverage server being copied into what Konflux perceives as the vendor directory, throwing off checksums. If go.mod is detected, stop the search for main packages.
|
@jupierce unit-tests gatekeeper is failing...looks like a lint/formating issue. PTAL |
jupierce
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
jupierce
force-pushed
the
coverage_stream
branch
from
93674a8 to
f4dce42
Compare
|
@jupierce: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Successful image / kernel build: https://art-jenkins.apps.prod-stable-spoke1-dc-iad2.itup.redhat.com/job/aos-cd-builds/job/build%252Focp4-konflux/25522/consoleFull .