-
Notifications
You must be signed in to change notification settings - Fork 13
✨ Overrides: allow appending/changing containers and initContainers #407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
To fascilitate downstream modifications, we need to support appending more containers to the deployment. For example, OpenShift uses sidecar containers to manage networking. Unless we want to hardcode all possible cases, we need to allow overrides for containers. This change adds two more fields to Overrides: containers and initContainers. By default, items are appended to the respective lists. If a name of an existing container is used, the existing container is replaced instead. The downside of the approach here is that both the manifests and the API documentation become huge with the addition of the entire Container structure from Kubernetes. Assisted-By: Claude Code (commercial license) Signed-off-by: Dmitry Tantsur <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Rozzii The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for overriding containers and initContainers in Ironic deployments to facilitate downstream modifications like OpenShift's sidecar containers. The implementation allows both appending new containers and replacing existing ones by name.
Key changes:
- Added
ContainersandInitContainersfields to theOverridesstruct - Implemented
applyContainerOverridesfunction with replace-or-append logic - Added comprehensive test coverage for various override scenarios
Reviewed Changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v1alpha1/ironic_types.go | Added Containers and InitContainers fields to Overrides struct with documentation |
| api/v1alpha1/zz_generated.deepcopy.go | Generated DeepCopy methods for new container fields and fixed import alias |
| api/go.mod | Added k8s.io/api dependency for Container types |
| pkg/ironic/utils.go | Implemented applyContainerOverrides function and integrated it into applyOverridesToPod |
| pkg/ironic/utils_test.go | Added comprehensive test cases for container override logic |
Comments suppressed due to low confidence (1)
api/go.mod:3
- Go 1.24.0 does not exist. As of January 2025, the latest stable Go version is 1.23.x. This version should be corrected to a valid Go version such as 1.23.0 or an earlier stable release.
go 1.24.0
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Containers to append to the main Ironic pod. | ||
| // If a container name matches an existing container, the existing container is replaced. |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The documentation should clarify the order of operations: containers are first checked for replacement, then new containers are appended. Consider rephrasing to: 'Containers to add or replace in the main Ironic pod. Containers with names matching existing containers will replace them; others will be appended.'
| // Containers to append to the main Ironic pod. | |
| // If a container name matches an existing container, the existing container is replaced. | |
| // Containers to add or replace in the main Ironic pod. | |
| // Containers with names matching existing containers will replace them; others will be appended. |
| // InitContainers to append to the main Ironic pod. | ||
| // If a container name matches an existing init container, the existing init container is replaced. |
Copilot
AI
Oct 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The documentation should clarify the order of operations: init containers are first checked for replacement, then new init containers are appended. Consider rephrasing to: 'InitContainers to add or replace in the main Ironic pod. Containers with names matching existing init containers will replace them; others will be appended.'
| // InitContainers to append to the main Ironic pod. | |
| // If a container name matches an existing init container, the existing init container is replaced. | |
| // InitContainers to add or replace in the main Ironic pod. | |
| // Containers with names matching existing init containers will replace them; others will be appended. |
|
/lgtm |
To fascilitate downstream modifications, we need to support appending
more containers to the deployment. For example, OpenShift uses sidecar
containers to manage networking. Unless we want to hardcode all possible
cases, we need to allow overrides for containers.
This change adds two more fields to Overrides: containers and
initContainers. By default, items are appended to the respective lists.
If a name of an existing container is used, the existing container is
replaced instead.
The downside of the approach here is that both the manifests and the API
documentation become huge with the addition of the entire Container
structure from Kubernetes.
Assisted-By: Claude Code (commercial license)
Signed-off-by: Dmitry Tantsur [email protected]