Add TTL fields to Build and BuildConfig APIs #2698
Open
Conversation
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
This commit implements time-based automatic cleanup for Build objects by adding TTL (time-to-live) fields to both BuildSpec and BuildConfigSpec, following the same pattern as Kubernetes Jobs' ttlSecondsAfterFinished. Changes: - Add SuccessfulBuildTTLSeconds and FailedBuildTTLSeconds to BuildSpec - Add DefaultSuccessfulBuildTTLSeconds and DefaultFailedBuildTTLSeconds to BuildConfigSpec - Regenerate deepcopy, swagger, and OpenAPI code This addresses issue openshift#2550: openshift#2550 The actual TTL deletion logic will be implemented in a separate PR in the build controller repository.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
size/M
Contributor
|
[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 |
📝 WalkthroughWalkthroughThis change introduces time-based retention fields to the Build and BuildConfig APIs. Four new optional integer pointer fields are added: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Hello @kairosci! Some important instructions when contributing to openshift/api: |
Review Summary by QodoAdd TTL fields to Build and BuildConfig APIs
WalkthroughsDescription• Add TTL fields to Build and BuildConfig APIs for time-based cleanup • Implement successfulBuildTTLSeconds and failedBuildTTLSeconds in BuildSpec • Implement defaultSuccessfulBuildTTLSeconds and defaultFailedBuildTTLSeconds in BuildConfigSpec • Regenerate deepcopy, swagger, and OpenAPI code for new fields Diagramflowchart LR
BuildSpec["BuildSpec"]
BuildConfigSpec["BuildConfigSpec"]
TTLFields["TTL Fields<br/>successfulBuildTTLSeconds<br/>failedBuildTTLSeconds"]
DefaultTTLFields["Default TTL Fields<br/>defaultSuccessfulBuildTTLSeconds<br/>defaultFailedBuildTTLSeconds"]
DeepCopy["DeepCopy Functions"]
Swagger["Swagger Documentation"]
OpenAPI["OpenAPI Schema"]
BuildSpec -- "adds" --> TTLFields
BuildConfigSpec -- "adds" --> DefaultTTLFields
TTLFields -- "regenerates" --> DeepCopy
DefaultTTLFields -- "regenerates" --> DeepCopy
DeepCopy -- "updates" --> Swagger
DeepCopy -- "updates" --> OpenAPI
File Changes1. build/v1/types.go
|
openshift-ci
bot
added
the
needs-ok-to-test
Contributor
|
Hi @kairosci. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Code Review by Qodo
1. generated.proto missing TTL fields
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+46
to
+58
| // successfulBuildTTLSeconds defines how long (in seconds) a successful build | ||
| // is retained after completion before being automatically deleted. | ||
| // If not set, the build will not be automatically deleted. | ||
| // This mirrors the semantics of Kubernetes Job's ttlSecondsAfterFinished. | ||
| // +optional | ||
| SuccessfulBuildTTLSeconds *int32 `json:"successfulBuildTTLSeconds,omitempty" protobuf:"varint,3,opt,name=successfulBuildTTLSeconds"` | ||
|
|
||
| // failedBuildTTLSeconds defines how long (in seconds) a failed or errored build | ||
| // is retained after completion before being automatically deleted. | ||
| // If not set, the build will not be automatically deleted. | ||
| // This mirrors the semantics of Kubernetes Job's ttlSecondsAfterFinished. | ||
| // +optional | ||
| FailedBuildTTLSeconds *int32 `json:"failedBuildTTLSeconds,omitempty" protobuf:"varint,4,opt,name=failedBuildTTLSeconds"` |
There was a problem hiding this comment.
1. generated.proto missing ttl fields 📘 Rule violation ✓ Correctness
• BuildSpec/BuildConfigSpec gained new TTL fields with protobuf tags, but the protobuf schema (generated.proto) does not include them. • This means protobuf clients/code that rely on generated.proto/generated.pb.go will not see or serialize the new fields, causing API/schema drift. • This indicates generated artifacts were not fully regenerated/synchronized prior to commit.
Agent Prompt
## Issue description
The new TTL API fields were added to `BuildSpec`/`BuildConfigSpec` with protobuf tags, but the protobuf-generated schema/code was not updated, leaving `generated.proto` (and likely `generated.pb.go`) out of sync.
## Issue Context
This repository commits generated artifacts. When Go types change (especially with protobuf tags), the corresponding protobuf outputs must be regenerated and committed so clients and tooling remain consistent.
## Fix Focus Areas
- build/v1/types.go[46-58]
- build/v1/types.go[1012-1028]
- build/v1/generated.proto[136-161]
- build/v1/generated.proto[455-464]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+46
to
+58
| // successfulBuildTTLSeconds defines how long (in seconds) a successful build | ||
| // is retained after completion before being automatically deleted. | ||
| // If not set, the build will not be automatically deleted. | ||
| // This mirrors the semantics of Kubernetes Job's ttlSecondsAfterFinished. | ||
| // +optional | ||
| SuccessfulBuildTTLSeconds *int32 `json:"successfulBuildTTLSeconds,omitempty" protobuf:"varint,3,opt,name=successfulBuildTTLSeconds"` | ||
|
|
||
| // failedBuildTTLSeconds defines how long (in seconds) a failed or errored build | ||
| // is retained after completion before being automatically deleted. | ||
| // If not set, the build will not be automatically deleted. | ||
| // This mirrors the semantics of Kubernetes Job's ttlSecondsAfterFinished. | ||
| // +optional | ||
| FailedBuildTTLSeconds *int32 `json:"failedBuildTTLSeconds,omitempty" protobuf:"varint,4,opt,name=failedBuildTTLSeconds"` |
There was a problem hiding this comment.
2. Ttl fields lack featuregate 📘 Rule violation ⛯ Reliability
• New fields were added to stable build/v1 APIs without a +openshift:enable:FeatureGate=... marker. • Without feature-gating, clusters/profiles cannot control rollout of this stable API change, violating the repository’s stability/compatibility policy for new v1 fields. • No corresponding FeatureGate registration for a Build TTL feature exists in features/features.go using the required builder pattern.
Agent Prompt
## Issue description
The new TTL fields were added to stable `build/v1` APIs without FeatureGate markers, and there is no registered FeatureGate for this capability in `features/features.go`.
## Issue Context
Repository policy requires stable API field additions to be gated via `+openshift:enable:FeatureGate=...`, and any new FeatureGate must be registered using the builder pattern with required metadata.
## Fix Focus Areas
- build/v1/types.go[46-58]
- build/v1/types.go[1012-1028]
- features/features.go[36-90]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1012
to
+1028
| // defaultSuccessfulBuildTTLSeconds sets the default retention time (in seconds) | ||
| // for successful builds created from this BuildConfig. | ||
| // Builds created from this BuildConfig will inherit this value unless overridden | ||
| // in the Build's own successfulBuildTTLSeconds field. | ||
| // If not set, builds will not have an automatic TTL set. | ||
| // This mirrors the semantics of Kubernetes Job's ttlSecondsAfterFinished. | ||
| // +optional | ||
| DefaultSuccessfulBuildTTLSeconds *int32 `json:"defaultSuccessfulBuildTTLSeconds,omitempty" protobuf:"varint,6,opt,name=defaultSuccessfulBuildTTLSeconds"` | ||
|
|
||
| // defaultFailedBuildTTLSeconds sets the default retention time (in seconds) | ||
| // for failed or errored builds created from this BuildConfig. | ||
| // Builds created from this BuildConfig will inherit this value unless overridden | ||
| // in the Build's own failedBuildTTLSeconds field. | ||
| // If not set, builds will not have an automatic TTL set. | ||
| // This mirrors the semantics of Kubernetes Job's ttlSecondsAfterFinished. | ||
| // +optional | ||
| DefaultFailedBuildTTLSeconds *int32 `json:"defaultFailedBuildTTLSeconds,omitempty" protobuf:"varint,7,opt,name=defaultFailedBuildTTLSeconds"` |
There was a problem hiding this comment.
3. No ttl integration validation tests 📘 Rule violation ⛯ Reliability
• The PR changes the build/v1 API schema but does not add integration validation tests under the repository’s /<group>/<version>/tests/<crd-name>/ structure. • Without these tests, schema expectations for the new TTL fields (presence, optionality, gating expectations) are not enforced and regressions are more likely.
Agent Prompt
## Issue description
API schema was extended with TTL fields but no integration validation tests were added under the expected `/<group>/<version>/tests/<crd-name>/` structure.
## Issue Context
The repository uses YAML-based integration schema tests (see `operator/v1/tests/...`). Similar tests should be added/updated to ensure the new TTL fields are validated in generated schemas and remain stable over time.
## Fix Focus Areas
- build/v1/types.go[46-58]
- build/v1/types.go[1012-1028]
- operator/v1/tests/networks.operator.openshift.io/AAA_ungated.yaml[1-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Summary
This PR implements time-based automatic cleanup for Build objects by adding TTL (time-to-live) fields to both and , following the same pattern as Kubernetes Jobs'
ttlSecondsAfterFinished.Changes
API Changes
successfulBuildTTLSecondsandfailedBuildTTLSecondstoBuildSpecdefaultSuccessfulBuildTTLSecondsanddefaultFailedBuildTTLSecondstoBuildConfigSpecGenerated Code
Motivation
This addresses issue #2550. Currently, OpenShift administrators need to implement custom automation (e.g., cronjobs or external scripts) to prune old builds. While OpenShift provides
successfulBuildsHistoryLimitandfailedBuildsHistoryLimit, those are count-based, not time-based.This PR extends both Build and BuildConfig objects to include time-based retention semantics, providing more predictable and automated cleanup.
Backward Compatibility
All new fields are optional (
*int32pointer types withomitemptyJSON tags), ensuring full backward compatibility.Next Steps
The actual TTL deletion logic will need to be implemented in a separate PR in the build controller repository.
Testing
Fixes #2550