Modifying build client to handle different build logics#5224
Modifying build client to handle different build logics#5224hector-vido wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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 the 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 | ✅ 4 | ❌ 13❌ Failed checks (1 warning, 12 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Linked repositories: Couldn't analyze Linked repositories: Couldn't analyze Linked repositories: Couldn't analyze Git: Failed to clone repository. Please run the Comment |
|
[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
| } | ||
|
|
||
| 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 { |
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.
2 participants
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