Upgrade OpenshiftClient to 7.4.0 #632
Merged
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 GuideUpgrades Fabric8 OpenShift client to 7.4.0, removes the deprecated server-mock in favor of Mockito-based tests, adjusts Docker image metadata handling to the new API, and refactors tests to be pure unit tests or to use explicit client DSL mocking. Sequence diagram for DockerImageMetadata.getMetadataFromTag with new Fabric8 APIsequenceDiagram
participant Caller
participant DockerImageMetadata
participant ImageStreamTag
participant Image
participant GenericKubernetesResource
Caller->>DockerImageMetadata: getMetadataFromTag(imageStreamTag)
DockerImageMetadata->>DockerImageMetadata: areMetadataForImageReady(imageStreamTag)
alt metadata ready
DockerImageMetadata->>ImageStreamTag: getImage()
ImageStreamTag-->>DockerImageMetadata: Image
DockerImageMetadata->>Image: getDockerImageMetadata()
Image-->>DockerImageMetadata: GenericKubernetesResource
DockerImageMetadata->>GenericKubernetesResource: getAdditionalProperties()
GenericKubernetesResource-->>DockerImageMetadata: MapStringObject
DockerImageMetadata-->>Caller: DockerImageMetadata
else metadata not ready
DockerImageMetadata-->>Caller: null
end
Class diagram for updated DockerImageMetadata metadata handlingclassDiagram
class DockerImageMetadata {
+DockerImageMetadata(MapStringObject data)
+static DockerImageMetadata getMetadata(OpenShift openShift, Image image)
-static DockerImageMetadata getMetadataFromTag(ImageStreamTag imageStreamTag)
-static boolean areMetadataForImageReady(ImageStreamTag tag)
}
class ImageStreamTag {
+Image getImage()
}
class Image {
+GenericKubernetesResource getDockerImageMetadata()
}
class GenericKubernetesResource {
+MapStringObject getAdditionalProperties()
}
DockerImageMetadata ..> ImageStreamTag : uses
ImageStreamTag --> Image : contains
Image --> GenericKubernetesResource : dockerImageMetadata
DockerImageMetadata ..> GenericKubernetesResource : reads metadata
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 - I've found 1 issue, and left some high level feedback:
- In DockerImageMetadata.getMetadataFromTag, the unchecked cast to GenericKubernetesResource assumes the implementation type of getDockerImageMetadata(); consider guarding with an instanceof check (or handling alternative types) to avoid a potential ClassCastException if Fabric8 changes the underlying object again.
- BinaryBuildMemoryTest repeats System property cleanup and XTFConfig.loadConfig() both in @beforeeach and in each test’s finally block; centralizing this reset logic (e.g., in an @AfterEach helper) would reduce duplication and lower the risk of inconsistent configuration between tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DockerImageMetadata.getMetadataFromTag, the unchecked cast to GenericKubernetesResource assumes the implementation type of getDockerImageMetadata(); consider guarding with an instanceof check (or handling alternative types) to avoid a potential ClassCastException if Fabric8 changes the underlying object again.
- BinaryBuildMemoryTest repeats System property cleanup and XTFConfig.loadConfig() both in @BeforeEach and in each test’s finally block; centralizing this reset logic (e.g., in an @AfterEach helper) would reduce duplication and lower the risk of inconsistent configuration between tests.
## Individual Comments
### Comment 1
<location> `builder/src/test/java/cz/xtf/builder/openshift/smoke/BuildersTest.java:37-46` </location>
<code_context>
- // arrange
- final String dataEntryKey = "test.properties";
- final String dataEntryValue = "foo=bar";
- final ConfigMap configMap = new ConfigMapBuilder(TEST_CONFIGMAP_NAME)
- .addLabels(TEST_RESOURCE_LABELS)
- .configEntry(dataEntryKey, dataEntryValue)
</code_context>
<issue_to_address>
**suggestion (testing):** Extend builder tests to assert labels (and potentially triggers) to better cover builder behavior
Currently these tests only assert name/data for `ConfigMap` and a few fields for `DeploymentConfig`. Since labels are part of the builder contract (and you pass `TEST_RESOURCE_LABELS`), please also assert that these labels are present in `metadata.labels` for both resources. For `DeploymentConfigBuilder`, also verify that the image change trigger configured via `onImageChange()` appears in `.spec.triggers` with the expected type, so these tests fully cover the intended builder behavior and can reliably replace `BasicOpenShiftTest`.
Suggested implementation:
```java
Assertions.assertEquals(TEST_CONFIGMAP_NAME,
configMap.getMetadata().getName(),
"ConfigMap resource has unexpected name.");
Assertions.assertEquals(TEST_RESOURCE_LABELS,
configMap.getMetadata().getLabels(),
"ConfigMap resource has unexpected labels.");
```
To fully implement the requested coverage for the `DeploymentConfigBuilder` as per your review comment, make the following changes in the same test class, inside the `testDeploymentConfigBuilder` (or equivalent) method that builds and asserts the `DeploymentConfig`:
1. **Assert labels on the DeploymentConfig metadata**
After the existing assertion that checks the DeploymentConfig name (and before or after the replicas assertion), add:
```java
Assertions.assertEquals(TEST_RESOURCE_LABELS,
deploymentConfig.getMetadata().getLabels(),
"DeploymentConfig resource has unexpected labels.");
```
This assumes the variable holding the built resource is named `deploymentConfig`. Adjust the variable name if different.
2. **Assert the presence of the ImageChange trigger**
After the basic structural assertions (name, replicas, etc.), add assertions that verify the `ImageChange` trigger created by `.onImageChange()` is present in `.spec.triggers`:
```java
Assertions.assertNotNull(deploymentConfig.getSpec(), "DeploymentConfig spec should not be null.");
Assertions.assertNotNull(deploymentConfig.getSpec().getTriggers(), "DeploymentConfig triggers should not be null.");
boolean hasImageChangeTrigger = deploymentConfig.getSpec().getTriggers()
.stream()
.anyMatch(trigger -> "ImageChange".equals(trigger.getType()));
Assertions.assertTrue(hasImageChangeTrigger,
"DeploymentConfig should contain an ImageChange trigger configured via onImageChange().");
```
If your test currently doesn’t import `java.util.stream.*` or similar, no extra imports are needed because `stream()` is available on `List`. Ensure the `deploymentConfig` type is `io.fabric8.openshift.api.model.DeploymentConfig` (or compatible) so `getSpec().getTriggers()` and `getType()` on the trigger objects are available.
These additions will ensure the tests cover both label propagation and trigger configuration behavior for the builders, so they can reliably replace the previous `BasicOpenShiftTest` coverage.
</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
+37
to
+46
| final ConfigMap configMap = new ConfigMapBuilder(TEST_CONFIGMAP_NAME) | ||
| .addLabels(TEST_RESOURCE_LABELS) | ||
| .configEntry(dataEntryKey, dataEntryValue) | ||
| .build(); | ||
|
|
||
| // assert - verify the built ConfigMap structure | ||
| Assertions.assertNotNull(configMap, "ConfigMap resource creation failed."); | ||
| Assertions.assertEquals(TEST_CONFIGMAP_NAME, | ||
| configMap.getMetadata().getName(), | ||
| "ConfigMap resource has unexpected name."); |
There was a problem hiding this comment.
suggestion (testing): Extend builder tests to assert labels (and potentially triggers) to better cover builder behavior
Currently these tests only assert name/data for ConfigMap and a few fields for DeploymentConfig. Since labels are part of the builder contract (and you pass TEST_RESOURCE_LABELS), please also assert that these labels are present in metadata.labels for both resources. For DeploymentConfigBuilder, also verify that the image change trigger configured via onImageChange() appears in .spec.triggers with the expected type, so these tests fully cover the intended builder behavior and can reliably replace BasicOpenShiftTest.
Suggested implementation:
Assertions.assertEquals(TEST_CONFIGMAP_NAME,
configMap.getMetadata().getName(),
"ConfigMap resource has unexpected name.");
Assertions.assertEquals(TEST_RESOURCE_LABELS,
configMap.getMetadata().getLabels(),
"ConfigMap resource has unexpected labels.");To fully implement the requested coverage for the DeploymentConfigBuilder as per your review comment, make the following changes in the same test class, inside the testDeploymentConfigBuilder (or equivalent) method that builds and asserts the DeploymentConfig:
-
Assert labels on the DeploymentConfig metadata
After the existing assertion that checks the DeploymentConfig name (and before or after the replicas assertion), add:
Assertions.assertEquals(TEST_RESOURCE_LABELS, deploymentConfig.getMetadata().getLabels(), "DeploymentConfig resource has unexpected labels.");
This assumes the variable holding the built resource is named
deploymentConfig. Adjust the variable name if different. -
Assert the presence of the ImageChange trigger
After the basic structural assertions (name, replicas, etc.), add assertions that verify the
ImageChangetrigger created by.onImageChange()is present in.spec.triggers:Assertions.assertNotNull(deploymentConfig.getSpec(), "DeploymentConfig spec should not be null."); Assertions.assertNotNull(deploymentConfig.getSpec().getTriggers(), "DeploymentConfig triggers should not be null."); boolean hasImageChangeTrigger = deploymentConfig.getSpec().getTriggers() .stream() .anyMatch(trigger -> "ImageChange".equals(trigger.getType())); Assertions.assertTrue(hasImageChangeTrigger, "DeploymentConfig should contain an ImageChange trigger configured via onImageChange().");
If your test currently doesn’t import
java.util.stream.*or similar, no extra imports are needed becausestream()is available onList. Ensure thedeploymentConfigtype isio.fabric8.openshift.api.model.DeploymentConfig(or compatible) sogetSpec().getTriggers()andgetType()on the trigger objects are available.
These additions will ensure the tests cover both label propagation and trigger configuration behavior for the builders, so they can reliably replace the previous BasicOpenShiftTest coverage.
kanovotn
force-pushed
the
openshiftClient74
branch
from
b2c1173 to
49e5c83
Compare
kanovotn
force-pushed
the
openshiftClient74
branch
2 times, most recently
from
cad69d3 to
bc3e79f
Compare
kanovotn
force-pushed
the
openshiftClient74
branch
from
bc3e79f to
132dbcc
Compare
mnovak
approved these changes
Jan 14, 2026
Contributor
mnovak
left a comment
mnovak
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot for this work. Merging.
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.
Upgrade Fabric8 OpenShift Client to 7.4.0
Summary
Upgrades Fabric8 OpenShift Client from 6.14.0 to 7.4.0 and migrates affected tests.
Changes
Test Migrations
Summary by Sourcery
Upgrade Fabric8 OpenShift client to 7.4.0 and migrate dependent tests and utilities away from the removed server mock APIs.
Enhancements:
Build: