Modifying build client to handle different build logics#5224
Draft
hector-vido wants to merge 1 commit into
Draft
Modifying build client to handle different build logics#5224hector-vido wants to merge 1 commit into
hector-vido wants to merge 1 commit into
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughAdds a BuildType field and openshift constant to release config, defaults BuildType to "openshift", requires a buildType argument when constructing BuildClient, extends BuildClient with BuildType(), and refactors build execution to dispatch by build type with an extracted OpenShift-specific build helper. ChangesBuild-type addition and build execution refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hector-vido The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/steps/build_client.go`:
- Around line 16-19: The exported constant names are misspelled/inconsistent:
rename BuiltTypeOpenshift and BuilTypeAWSCodeBuild to BuildTypeOpenshift and
BuildTypeAWSCodeBuild respectively, update all usages (including the caller in
defaults.go and the Build() dispatch) to the new identifiers, and run
tests/compile to ensure no remaining references to the old names remain.
- Around line 41-51: The test failures come from calling NewBuildClient with the
old 6-arg signature; update each test call (e.g., in
pkg/defaults/defaults_test.go, pkg/steps/source_test.go,
pkg/steps/index_generator_test.go) to pass the new buildType as the second
argument to NewBuildClient and shift the remaining parameters right so the
restClient, nodeArchitectures, manifestToolDockerCfg, localRegistryDNS, and
metricsAgent match the new signature used in the NewBuildClient function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a3d85046-4e20-4c0d-9b63-9f2391f2bb2c
📒 Files selected for processing (2)
pkg/defaults/defaults.gopkg/steps/build_client.go
hector-vido
force-pushed
the
buildclient-adjustment
branch
from
3579519 to
3d53579
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/api/types.go (1)
78-78: ⚡ Quick winAdd a doc comment for
BuildTypeonReleaseBuildConfiguration.Line 78 introduces an exported API field without documenting purpose/allowed values/default behavior. Add a short field comment to keep config API self-describing.
As per coding guidelines: "
**/*.go: Go documentation on Classes/Functions/Fields should be written properly".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/types.go` at line 78, Add a concise Go doc comment above the exported BuildType field on the ReleaseBuildConfiguration struct explaining what the field represents, the allowed values (e.g., "debug", "release", "optimized" or whatever applies), and any default behavior when omitted; update the comment to be a single-line field comment starting with BuildType so Go tooling picks it up and keep phrasing brief and self-describing for API consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/defaults/defaults.go`:
- Line 63: The code is hardcoding api.BuildTypeOpenshift when constructing the
build client; change the call to steps.NewBuildClient to pass the configured
build type from cfg (e.g., cfg.BuildType) instead of api.BuildTypeOpenshift,
ensuring the value you pass matches the expected parameter type for
steps.NewBuildClient (cast to api.BuildType if necessary) so alternate build
implementations configured at runtime are respected.
---
Nitpick comments:
In `@pkg/api/types.go`:
- Line 78: Add a concise Go doc comment above the exported BuildType field on
the ReleaseBuildConfiguration struct explaining what the field represents, the
allowed values (e.g., "debug", "release", "optimized" or whatever applies), and
any default behavior when omitted; update the comment to be a single-line field
comment starting with BuildType so Go tooling picks it up and keep phrasing
brief and self-describing for API consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7067a433-9827-4b0c-9e30-c6cee8061db3
📒 Files selected for processing (5)
pkg/api/config.gopkg/api/types.gopkg/defaults/defaults.gopkg/steps/build_client.gopkg/steps/source.go
hector-vido
force-pushed
the
buildclient-adjustment
branch
from
3d53579 to
af741f2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/types.go (1)
78-78: ⚡ Quick winDocument the exported
BuildTypefield.Please add a field comment describing supported values/default behavior for
BuildType, since this is part of the user-facing config API.Suggested diff
- BuildType string `json:"build_type,omitempty"` + // BuildType selects the build implementation. Defaults to "openshift". + BuildType string `json:"build_type,omitempty"`As per coding guidelines, "
**/*.go: Go documentation on Classes/Functions/Fields should be written properly".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/types.go` at line 78, Add a Go doc comment for the exported BuildType field on the relevant struct (BuildType string `json:"build_type,omitempty"`) that describes the supported values (e.g., possible enum strings), the default behavior when omitted, and any validation/semantic meaning; place the comment immediately above the BuildType field so it appears in generated docs and follows Go doc conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/api/types.go`:
- Line 78: Add a Go doc comment for the exported BuildType field on the relevant
struct (BuildType string `json:"build_type,omitempty"`) that describes the
supported values (e.g., possible enum strings), the default behavior when
omitted, and any validation/semantic meaning; place the comment immediately
above the BuildType field so it appears in generated docs and follows Go doc
conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1d0f9413-0683-4e1f-a066-afe8035b0abf
📒 Files selected for processing (8)
pkg/api/config.gopkg/api/types.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/build_client.gopkg/steps/index_generator_test.gopkg/steps/source.gopkg/steps/source_test.go
droslean
reviewed
Jun 3, 2026
| } | ||
|
|
||
| func handleBuild(ctx context.Context, client BuildClient, podClient kubernetes.PodClient, build buildapi.Build) error { | ||
| func handleBuild(ctx context.Context, buildClient BuildClient, podClient kubernetes.PodClient, build buildapi.Build) error { |
Member
There was a problem hiding this comment.
But this handles only openshift builds already. I don't see any point of check the type in this layer. You should do it before this and don't make any changes here at all.
hector-vido
force-pushed
the
buildclient-adjustment
branch
from
af741f2 to
9b93b6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/steps/build_client.go (1)
24-24: 💤 Low valueConsider adding a doc comment for the new interface method.
The
BuildType()method is now part of the publicBuildClientinterface. A brief comment describing what build types are expected (e.g.,api.BuildTypeOpenshift) would help consumers.Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/steps/build_client.go` at line 24, Add a Go doc comment for the new BuildType() method on the BuildClient interface explaining what values it returns and what they represent (e.g., returns a string identifying the build platform/type such as api.BuildTypeOpenshift), so consumers know expected constants and semantics; update the comment above the BuildType() declaration in the BuildClient interface and mirror the same descriptive comment near the other occurrence (lines 79-81) where the method is declared or referenced.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/steps/source.go`:
- Around line 495-506: The code registers the same workload twice:
metricsAgent.AddNodeWorkload is called in the goroutine where builds are
dispatched and again inside handleOpenshiftBuild; remove the duplicate by
deleting the AddNodeWorkload call from the dispatching goroutine (the call shown
in the snippet) and leave the registration in handleOpenshiftBuild, ensuring
RemoveNodeWorkload(b.Name) remains paired for cleanup; keep the rest of the
error handling (err, errChan) and wg.Done as-is.
---
Nitpick comments:
In `@pkg/steps/build_client.go`:
- Line 24: Add a Go doc comment for the new BuildType() method on the
BuildClient interface explaining what values it returns and what they represent
(e.g., returns a string identifying the build platform/type such as
api.BuildTypeOpenshift), so consumers know expected constants and semantics;
update the comment above the BuildType() declaration in the BuildClient
interface and mirror the same descriptive comment near the other occurrence
(lines 79-81) where the method is declared or referenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 12449dbe-f6a8-4642-aa74-7497ea463776
📒 Files selected for processing (8)
pkg/api/config.gopkg/api/types.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/build_client.gopkg/steps/index_generator_test.gopkg/steps/source.gopkg/steps/source_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/defaults/defaults_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/defaults/defaults.go
- pkg/steps/index_generator_test.go
- pkg/api/config.go
- pkg/api/types.go
- pkg/steps/source_test.go
Comment on lines
+495
to
506
| var err error | ||
| defer wg.Done() | ||
| metricsAgent.AddNodeWorkload(ctx, b.Namespace, fmt.Sprintf("%s-build", b.Name), b.Name, podClient) | ||
| if err := handleBuild(ctx, buildClient, podClient, b); err != nil { | ||
| if buildClient.BuildType() == api.BuildTypeOpenshift { | ||
| err = handleOpenshiftBuild(ctx, buildClient, podClient, b) | ||
| } else { | ||
| err = fmt.Errorf("undefined build type %s", buildClient.BuildType()) | ||
| } | ||
| if err != nil { | ||
| errChan <- fmt.Errorf("error occurred handling build %s: %w", b.Name, err) | ||
| } | ||
| metricsAgent.RemoveNodeWorkload(b.Name) |
There was a problem hiding this comment.
Duplicate workload registration: AddNodeWorkload called twice.
Line 497 calls metricsAgent.AddNodeWorkload(...) before dispatching. Then handleOpenshiftBuild calls the same thing again at line 577. The workload gets registered twice for the same build.
Either remove the call here (lines 497) or remove it from handleOpenshiftBuild — but not both places.
🐛 Proposed fix: remove the call from handleBuilds
for _, build := range builds {
go func(b buildapi.Build) {
var err error
defer wg.Done()
- metricsAgent.AddNodeWorkload(ctx, b.Namespace, fmt.Sprintf("%s-build", b.Name), b.Name, podClient)
if buildClient.BuildType() == api.BuildTypeOpenshift {
err = handleOpenshiftBuild(ctx, buildClient, podClient, b)
} else {
err = fmt.Errorf("undefined build type %s", buildClient.BuildType())
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/steps/source.go` around lines 495 - 506, The code registers the same
workload twice: metricsAgent.AddNodeWorkload is called in the goroutine where
builds are dispatched and again inside handleOpenshiftBuild; remove the
duplicate by deleting the AddNodeWorkload call from the dispatching goroutine
(the call shown in the snippet) and leave the registration in
handleOpenshiftBuild, ensuring RemoveNodeWorkload(b.Name) remains paired for
cleanup; keep the rest of the error handling (err, errChan) and wg.Done as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the first step to isolate image build logic inside
buildClient, so it can handle different builds on different platforms like AWS, Shipwright, etc.Summary
This PR introduces a build-type abstraction into ci-operator’s build client so the system can host multiple build-platform implementations. It isolates image build logic inside buildClient and wires configuration through to the client so ci-operator can choose platform-specific build behavior (current default: OpenShift).
What changed (practical effect)
Component impact
Backwards compatibility and next steps
Notes for reviewers