Skip to content

refactor: update downstream Containerfile for Go rewrite - #346

Open
rm3l wants to merge 2 commits into
RHIDP-16554-6--remove-vendored-depsfrom
RHIDP-16554-7--update-downstream-containerfile
Open

refactor: update downstream Containerfile for Go rewrite#346
rm3l wants to merge 2 commits into
RHIDP-16554-6--remove-vendored-depsfrom
RHIDP-16554-7--update-downstream-containerfile

Conversation

@rm3l

@rm3l rm3l commented Sep 7, 2026

Copy link
Copy Markdown
Member

Description

Update the downstream Konflux Containerfile (.rhdh/docker/Containerfile) for the Go rewrite:

  • Replaced: websocat Rust builder stage, helm CGW binary installer stage, oc/kubectl installer, yq/pip installer → single Go builder stage with cachi2 gomod prefetch
  • Simplified: final image runtime deps from 12 packages (tar, gzip, bash, findutils, grep, sed, jq, python3, python3-pip, util-linux, rsync, shadow-utils) to 3 (tar, util-linux, rsync)
  • Removed: .rhdh/docker/requirements*.{in,txt} — pip requirements files for yq (no longer needed)
  • Removed: .pre-commit-config.yaml and 3 pre-commit CI workflows — all hooks existed solely to keep pip requirements in sync
  • Updated: CLAUDE.md — architecture, commands, key patterns, and downstream build sections now reflect the Go rewrite
  • Updated: CONTRIBUTING.md — removed pre-commit hooks and vendored dependencies sections

Part of the Go rewrite epic (RHIDP-16540).

Which issue(s) does this PR fix or relate to

PR acceptance criteria

  • Tests — make go-build, make go-test, make go-lint all pass
  • Documentation — CLAUDE.md and CONTRIBUTING.md updated

How to test changes / Special notes to the reviewer

The downstream Containerfile cannot be tested locally (requires cachi2 prefetch infrastructure). Verify the Containerfile structure matches the upstream pattern but with cachi2 gomod sourcing:

  • Go builder sources /cachi2/cachi2.env before go build
  • Final image RPM install sources /cachi2/cachi2.env and configures repos from prefetched rpms

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Sep 7, 2026
@rm3l
rm3l force-pushed the RHIDP-16554-7--update-downstream-containerfile branch 5 times, most recently from 112a6a1 to 58bb5c0 Compare September 7, 2026 21:29
@rm3l
rm3l marked this pull request as ready for review September 8, 2026 16:36
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Sep 8, 2026
@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Refactor downstream Containerfile for hermetic Go build

✨ Enhancement ⚙️ Configuration changes 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Replaces legacy tool builders and installers with a cachi2-backed Go build stage.
• Reduces the runtime image to the compiled gather binary and three required RPMs.
• Removes obsolete Python requirements and documents the Go-based architecture and workflow.
Diagram

graph TD
  SRC["Go Sources"] --> BUILD["Go Builder"] --> BIN["Gather Binary"] --> IMG["Runtime Image"]
  GOMOD["Cachi2 Go Modules"] --> BUILD
  RPM["Cachi2 RPMs"] --> BASE["UBI Minimal"] --> IMG
Loading
High-Level Assessment

The two-stage Go build is the appropriate approach because the rewritten application already embeds collection behavior through Go libraries. Retaining separate Rust, Helm, OpenShift client, and Python installation stages would duplicate capabilities, enlarge the runtime image, and complicate hermetic dependency management. Review should focus on Konflux cachi2 inputs and propagation of the version argument into both build and runtime stages.

Files changed (3) +54 / -217

Documentation (2) +35 / -124
CLAUDE.mdDocument the Go collector architecture and downstream build +33/-64

Document the Go collector architecture and downstream build

• Updates development commands, component descriptions, collection flow, tests, and collector guidance for the Go implementation. Replaces legacy vendoring guidance with the cachi2 gomod and RPM prefetch model while identifying the shell implementation as transitional.

CLAUDE.md

CONTRIBUTING.mdAlign contributor guidance with the Go rewrite +2/-60

Align contributor guidance with the Go rewrite

• Updates local-run prerequisites and removes instructions for script-specific execution, pip synchronization hooks, and vendored external dependencies. Keeps image-building guidance while eliminating obsolete maintenance workflows.

CONTRIBUTING.md

Other (1) +19 / -93
ContainerfileReplace legacy toolchain stages with a hermetic Go build +19/-93

Replace legacy toolchain stages with a hermetic Go build

• Reworks the downstream image into a Go builder and minimal UBI runtime stage using cachi2-prefetched Go modules and RPMs. It compiles the release version into the gather binary, removes external tool installation, limits runtime packages to tar, util-linux, and rsync, and retains non-root execution.

.rhdh/docker/Containerfile

@rm3l
rm3l force-pushed the RHIDP-16554-7--update-downstream-containerfile branch from 58bb5c0 to ea2a0f2 Compare September 8, 2026 16:40
@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Sep 8, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (1) 📜 Skill insights (0)

Grey Divider


Action required

1. Chart log limits are ignored 🔗 Cross-repo conflict ≡ Correctness
Description
CollectPodLogs and writeAggregatedLogs create PodLogOptions without applying
MUST_GATHER_SINCE or MUST_GATHER_SINCE_TIME, even though the new image replaces the script that
previously honored them. When rhdh-chart users configure gather.since or gather.sinceTime, the
chart still supplies those variables but the rewritten image collects unrestricted logs instead.
Code

.rhdh/docker/Containerfile[R73-74]

+# Copy Go gather binary — all collection logic is built in, no external tools needed
+COPY --from=go-builder /tmp/gather /usr/bin/gather
Relevance

●●● Strong

Clear regression: configured log windows are ignored after replacing scripts with the Go collector.

PR-#137

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed Containerfile installs the Go binary as the image entrypoint, while rhdh-chart continues
translating its documented time-limit values into MUST_GATHER_SINCE and MUST_GATHER_SINCE_TIME.
The Go implementation reads those values only for summary text and constructs both individual and
aggregated Kubernetes log requests without either time option; the replaced script demonstrates that
these variables previously controlled collection.

.rhdh/docker/Containerfile[73-82]
internal/collector/pod.go[141-169]
internal/collector/operator.go[591-601]
internal/collector/namespace_inspect.go[242-244]
collection-scripts/common.sh[1990-2002]
External repo: redhat-developer/rhdh-chart, charts/must-gather/templates/deployment.yaml [57-69]
External repo: redhat-developer/rhdh-chart, charts/must-gather/values.yaml [66-75]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Preserve the rhdh-chart log-filtering contract in the Go gather implementation. Parse `MUST_GATHER_SINCE` and `MUST_GATHER_SINCE_TIME` and populate Kubernetes `PodLogOptions.SinceSeconds` or `SinceTime` for every current and previous log request, retaining the established precedence when both are present.

## Issue Context
The chart maps `gather.since` and `gather.sinceTime` to these environment variables. The legacy script applied them to log collection, but the Go binary activated by this Containerfile only prints them in a summary.

## Fix Focus Areas
- internal/collector/pod.go[141-169]
- internal/collector/operator.go[591-601]
- internal/collector/namespace_inspect.go[242-244]
- collection-scripts/common.sh[1990-2002]
- .rhdh/docker/Containerfile[73-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 7 rules
✅ Cross-repo context — repo relationships
  Explored: repo: redhat-developer/rhdh-chart (sha: 3ac5278f)
Review mode: ⚖️ Balanced: This changes a hermetic production Containerfile and replaces multiple runtime/build paths with a Go build, creating meaningful packaging, dependency, and runtime behavior risk.

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment on lines +73 to +74
# Copy Go gather binary — all collection logic is built in, no external tools needed
COPY --from=go-builder /tmp/gather /usr/bin/gather

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Chart log limits are ignored 🔗 Cross-repo conflict ≡ Correctness

CollectPodLogs and writeAggregatedLogs create PodLogOptions without applying
MUST_GATHER_SINCE or MUST_GATHER_SINCE_TIME, even though the new image replaces the script that
previously honored them. When rhdh-chart users configure gather.since or gather.sinceTime, the
chart still supplies those variables but the rewritten image collects unrestricted logs instead.
Agent Prompt
## Issue description
Preserve the rhdh-chart log-filtering contract in the Go gather implementation. Parse `MUST_GATHER_SINCE` and `MUST_GATHER_SINCE_TIME` and populate Kubernetes `PodLogOptions.SinceSeconds` or `SinceTime` for every current and previous log request, retaining the established precedence when both are present.

## Issue Context
The chart maps `gather.since` and `gather.sinceTime` to these environment variables. The legacy script applied them to log collection, but the Go binary activated by this Containerfile only prints them in a summary.

## Fix Focus Areas
- internal/collector/pod.go[141-169]
- internal/collector/operator.go[591-601]
- internal/collector/namespace_inspect.go[242-244]
- collection-scripts/common.sh[1990-2002]
- .rhdh/docker/Containerfile[73-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

MUST_GATHER_SINCE and MUST_GATHER_SINCE_TIME are only printed in the summary output, matching the original bash behavior. They were never applied to PodLogOptions in the original scripts either — not a regression.

@rm3l
rm3l force-pushed the RHIDP-16554-7--update-downstream-containerfile branch from ea2a0f2 to d480c68 Compare September 8, 2026 17:28
rm3l added 2 commits September 9, 2026 16:41
Replace the multi-stage Konflux Containerfile (websocat Rust builder,
helm CGW binary installer, oc/kubectl installer, yq/pip installer)
with a two-stage build: Go builder with cachi2 gomod prefetch and a
minimal final image needing only tar, util-linux, and rsync.

Remove pip requirements files, pre-commit hooks and their three CI
workflows (all existed solely to keep yq pip requirements in sync),
and update CLAUDE.md and CONTRIBUTING.md to reflect the new
architecture.

Assisted-by: Claude
Without declaring RHDH_MUST_GATHER_VERSION as an ARG in the go-builder
stage, the Docker variable substitution falls back to the default
value (0.0.0-unknown), ignoring the sync-midstream sed substitution
that only sets the ARG in the final stage. Adding the ARG in both
stages ensures the correct version is compiled into the Go binary.

Assisted-by: Claude
@rm3l
rm3l force-pushed the RHIDP-16554-7--update-downstream-containerfile branch from d480c68 to 5934a66 Compare September 9, 2026 14:41
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.

1 participant