[issue_624] Add configurable memory requests and limits for BuildConfigs #625
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
Reviewer's GuideThis PR adds optional, configurable memory requests and limits for BuildConfigs by extending the configuration and build classes, enhances failure detection, includes comprehensive tests and necessary test dependencies, and adds AI-assisted development guidelines. Entity relationship diagram for BuildConfig resource requirementserDiagram
BUILD_CONFIG {
string id
string builderImage
map envProperties
string memoryRequest
string memoryLimit
}
RESOURCE_REQUIREMENTS {
string memoryRequest
string memoryLimit
}
BUILD_CONFIG ||--|| RESOURCE_REQUIREMENTS : applies
Class diagram for updated BinaryBuild and BinaryBuildFromSourcesclassDiagram
class BuildManagerConfig {
+static String memoryRequest()
+static String memoryLimit()
}
class BinaryBuild {
-String memoryRequest
-String memoryLimit
+BinaryBuild(String builderImage, Path path, Map<String, String> envProperties, String id)
+BinaryBuild(String builderImage, Path path, Map<String, String> envProperties, String id, String memoryRequest, String memoryLimit)
-BuildConfig createBcDefinition()
}
class BinaryBuildFromSources {
+BinaryBuildFromSources(String builderImage, Path path, Map<String, String> envProperties, String id)
+BinaryBuildFromSources(String builderImage, Path path, Map<String, String> envProperties, String id, String memoryRequest, String memoryLimit)
}
BinaryBuildFromSources --|> BinaryBuild
BinaryBuild ..> BuildManagerConfig : uses
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the resource‐requirements construction into a shared helper method to avoid duplication and improve maintainability.
- Verify whether other build implementations (e.g. GitBuild) should also accept memory request/limit parameters or document why they remain unchanged.
- Replace raw phase strings like "Failed" and "Error" with constants or an enum to centralize phase definitions and prevent typos.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the resource‐requirements construction into a shared helper method to avoid duplication and improve maintainability.
- Verify whether other build implementations (e.g. GitBuild) should also accept memory request/limit parameters or document why they remain unchanged.
- Replace raw phase strings like "Failed" and "Error" with constants or an enum to centralize phase definitions and prevent typos.
## Individual Comments
### Comment 1
<location> `core/src/main/java/cz/xtf/core/bm/BinaryBuild.java:172-177` </location>
<code_context>
Build activeBuild = openShift.getBuild(id + "-" + activeBc.getStatus().getLastVersion());
if (activeBuild == null || activeBuild.getStatus() == null
- || "Failed".equals(activeBuild.getStatus().getPhase())) {
+ || "Failed".equals(activeBuild.getStatus().getPhase())
+ || "Error".equals(activeBuild.getStatus().getPhase())) {
log.debug("Build failed");
needsUpdate = true;
</code_context>
<issue_to_address>
**suggestion:** Additional build phase 'Error' is now considered for update, but other failure phases may exist.
Please review all possible build phases in OpenShift to ensure the update logic handles all terminal or problematic states, not just 'Failed' and 'Error'.
```suggestion
// List of problematic/terminal build phases according to OpenShift docs
Set<String> problematicPhases = new HashSet<>(Arrays.asList(
"Failed", "Error", "Cancelled"
));
if (activeBuild == null || activeBuild.getStatus() == null
|| problematicPhases.contains(activeBuild.getStatus().getPhase())) {
log.debug("Build failed or in problematic phase: {}",
activeBuild != null && activeBuild.getStatus() != null ? activeBuild.getStatus().getPhase() : "unknown");
needsUpdate = true;
}
```
</issue_to_address>
### Comment 2
<location> `core/src/test/java/cz/xtf/core/bm/BinaryBuildTest.java:71-80` </location>
<code_context>
+ openshift = OpenShifts.master(); // NOT static field!
+ }
+
+ @Test
+ void testDeployment() {
+ // Use builders to create resources
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid memory values.
Add test cases with invalid or malformed memory values to ensure the system responds appropriately, such as by ignoring them or raising clear exceptions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
172
to
177
| if (activeBuild == null || activeBuild.getStatus() == null | ||
| || "Failed".equals(activeBuild.getStatus().getPhase())) { | ||
| || "Failed".equals(activeBuild.getStatus().getPhase()) | ||
| || "Error".equals(activeBuild.getStatus().getPhase())) { | ||
| log.debug("Build failed"); | ||
| needsUpdate = true; | ||
| } |
There was a problem hiding this comment.
suggestion: Additional build phase 'Error' is now considered for update, but other failure phases may exist.
Please review all possible build phases in OpenShift to ensure the update logic handles all terminal or problematic states, not just 'Failed' and 'Error'.
Suggested change
| if (activeBuild == null || activeBuild.getStatus() == null | |
| || "Failed".equals(activeBuild.getStatus().getPhase())) { | |
| || "Failed".equals(activeBuild.getStatus().getPhase()) | |
| || "Error".equals(activeBuild.getStatus().getPhase())) { | |
| log.debug("Build failed"); | |
| needsUpdate = true; | |
| } | |
| // List of problematic/terminal build phases according to OpenShift docs | |
| Set<String> problematicPhases = new HashSet<>(Arrays.asList( | |
| "Failed", "Error", "Cancelled" | |
| )); | |
| if (activeBuild == null || activeBuild.getStatus() == null | |
| || problematicPhases.contains(activeBuild.getStatus().getPhase())) { | |
| log.debug("Build failed or in problematic phase: {}", | |
| activeBuild != null && activeBuild.getStatus() != null ? activeBuild.getStatus().getPhase() : "unknown"); | |
| needsUpdate = true; | |
| } |
Comment on lines
71
to
80
| @Test | ||
| public void testNeedsUpdate_WhenBuildStatusIsError_ShouldReturnTrue() { | ||
| // Given: BuildConfig and ImageStream exist with a build in "Error" status | ||
| ImageStream imageStream = createImageStream(TEST_BUILD_ID); | ||
| BuildConfig buildConfig = createBuildConfig(TEST_BUILD_ID, 1); | ||
| Build build = createBuildWithStatus(TEST_BUILD_ID + "-1", "Error"); | ||
|
|
||
| openShift.imageStreams().create(imageStream); | ||
| openShift.buildConfigs().create(buildConfig); | ||
| openShift.builds().create(build); |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for invalid memory values.
Add test cases with invalid or malformed memory values to ensure the system responds appropriately, such as by ignoring them or raising clear exceptions.
mchoma
force-pushed
the
issue_624_bc_resource_limits
branch
2 times, most recently
from
c90f11d to
db79361
Compare
mnovak
reviewed
Nov 11, 2025
CLAUDE.md
Outdated
| @@ -0,0 +1,209 @@ | |||
| # CLAUDE.md | |||
Contributor
There was a problem hiding this comment.
Could it be added in a separate PR?
Contributor
Author
There was a problem hiding this comment.
Yes, it just leaked from working directory into this PR
This change adds support for setting memory resource requirements on BuildConfigs through XTF configuration properties. Changes: - Added xtf.bm.memory.request and xtf.bm.memory.limit configuration properties to BuildManagerConfig - Updated BinaryBuild to apply ResourceRequirements when creating BuildConfig spec - Extended BinarySourceBuild constructor to accept memory parameters - Added comprehensive tests for memory configuration scenarios Memory configuration is optional and backward compatible. When not set, builds work as before without resource constraints. Example usage in test.properties: xtf.bm.memory.request=2Gi xtf.bm.memory.limit=4Gi 🤖 Generated with Claude Code on behalf of mchoma
mchoma
force-pushed
the
issue_624_bc_resource_limits
branch
from
db79361 to
69ede18
Compare
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 adds support for configuring memory resource requirements on BuildConfigs through XTF configuration properties.
Changes
xtf.bm.memory.requestandxtf.bm.memory.limitconfiguration propertiesUsage
Configure memory limits in
test.propertiesorglobal-test.properties:Backward Compatibility
Memory configuration is optional and backward compatible. When properties are not set, builds work as before without resource constraints.
Testing
All existing tests pass, and new tests verify:
🤖 Generated with Claude Code on behalf of the current user
Summary by Sourcery
Add configurable memory resource settings to binary builds via XTF configuration properties, update build logic to apply and detect resource constraints, include tests for various memory scenarios and error statuses, and provide AI-assisted development guidelines.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: