[issue_627] Fail fast when OpenShift admin token is not configured for Helm #629
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 guide (collapsed on small PRs)Reviewer's GuideAdds fail-fast validation in HelmBinaryManager to ensure OpenShift admin and master tokens are configured before constructing Helm binaries, throwing clear configuration errors when missing. Sequence diagram for fail-fast Helm admin binary creationsequenceDiagram
participant Caller
participant HelmBinaryManager
participant OpenShiftConfig
participant HelmBinary
Caller->>HelmBinaryManager: adminBinary()
HelmBinaryManager->>OpenShiftConfig: adminToken()
OpenShiftConfig-->>HelmBinaryManager: adminToken
HelmBinaryManager->>HelmBinaryManager: validateToken(adminToken, OPENSHIFT_ADMIN_TOKEN)
alt token is null
HelmBinaryManager-->>Caller: throw IllegalStateException
else token is valid
HelmBinaryManager->>HelmBinaryManager: getBinary(validAdminToken, namespace)
HelmBinaryManager-->>Caller: HelmBinary
end
Sequence diagram for fail-fast Helm master binary creationsequenceDiagram
participant Caller
participant HelmBinaryManager
participant OpenShiftConfig
participant HelmBinary
Caller->>HelmBinaryManager: masterBinary()
HelmBinaryManager->>OpenShiftConfig: masterToken()
OpenShiftConfig-->>HelmBinaryManager: masterToken
HelmBinaryManager->>HelmBinaryManager: validateToken(masterToken, OPENSHIFT_MASTER_TOKEN)
alt token is null
HelmBinaryManager-->>Caller: throw IllegalStateException
else token is valid
HelmBinaryManager->>HelmBinaryManager: getBinary(validMasterToken, namespace)
HelmBinaryManager-->>Caller: HelmBinary
end
Updated class diagram for HelmBinaryManager fail-fast validationclassDiagram
class HelmBinaryManager {
+HelmBinary adminBinary()
+HelmBinary masterBinary()
-String validateToken(String token, String propertyName)
-static HelmBinary getBinary(String token, String namespace)
+String getHelmBinaryPath()
}
class OpenShiftConfig {
+static String adminToken()
+static String masterToken()
+static String namespace()
+static String OPENSHIFT_ADMIN_TOKEN
+static String OPENSHIFT_MASTER_TOKEN
}
class HelmBinary {
}
HelmBinaryManager ..> OpenShiftConfig : uses
HelmBinaryManager ..> HelmBinary : creates
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:
- In both
adminBinary()andmasterBinary(), you retrieve the token into a local variable for validation but then callOpenShiftConfig.*Token()again when passing it togetBinary; consider reusing the validated local variable to avoid redundant calls and potential inconsistency. - The token checks only guard against
null; if an empty or whitespace-only string is also invalid in this context, consider tightening the validation (e.g., using a helper that checks for non-empty tokens) so failures are caught earlier. - The admin and master token validation logic is nearly identical; consider extracting a small helper to reduce duplication and keep error handling consistent for future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In both `adminBinary()` and `masterBinary()`, you retrieve the token into a local variable for validation but then call `OpenShiftConfig.*Token()` again when passing it to `getBinary`; consider reusing the validated local variable to avoid redundant calls and potential inconsistency.
- The token checks only guard against `null`; if an empty or whitespace-only string is also invalid in this context, consider tightening the validation (e.g., using a helper that checks for non-empty tokens) so failures are caught earlier.
- The admin and master token validation logic is nearly identical; consider extracting a small helper to reduce duplication and keep error handling consistent for future changes.
## Individual Comments
### Comment 1
<location> `core/src/main/java/cz/xtf/core/helm/HelmBinaryManager.java:25` </location>
<code_context>
}
public HelmBinary adminBinary() {
+ String adminToken = OpenShiftConfig.adminToken();
+ if (adminToken == null) {
+ throw new IllegalStateException(
+ "Admin token is not configured. Please set '" + OpenShiftConfig.OPENSHIFT_ADMIN_TOKEN
+ + "' in your properties file.");
+ }
return getBinary(OpenShiftConfig.adminToken(), OpenShiftConfig.namespace());
}
</code_context>
<issue_to_address>
**suggestion:** Use the already-fetched adminToken instead of calling OpenShiftConfig.adminToken() again.
`adminToken` is read into a local variable for the null check, but `getBinary` calls `OpenShiftConfig.adminToken()` again. This can create a small inconsistency window if the config changes, and the second call could still return `null`. Use the local `adminToken` for both the check and the `getBinary` call to keep the logic correct and consistent.
```suggestion
return getBinary(adminToken, OpenShiftConfig.namespace());
```
</issue_to_address>
### Comment 2
<location> `core/src/main/java/cz/xtf/core/helm/HelmBinaryManager.java:35` </location>
<code_context>
}
public HelmBinary masterBinary() {
+ String masterToken = OpenShiftConfig.masterToken();
+ if (masterToken == null) {
+ throw new IllegalStateException(
+ "Master token is not configured. Please set '" + OpenShiftConfig.OPENSHIFT_MASTER_TOKEN
+ + "' in your properties file.");
+ }
return getBinary(OpenShiftConfig.masterToken(), OpenShiftConfig.namespace());
}
</code_context>
<issue_to_address>
**suggestion:** Reuse the masterToken variable instead of calling OpenShiftConfig.masterToken() twice.
Right now you null-check the local `masterToken` but then call `OpenShiftConfig.masterToken()` again when invoking `getBinary`. Pass the local `masterToken` instead so the checked value is the one actually used and you don’t risk divergence if the config changes between calls.
```suggestion
return getBinary(masterToken, OpenShiftConfig.namespace());
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kanovotn
force-pushed
the
missingOpenshiftToken
branch
from
fef3766 to
6ec0bd4
Compare
Contributor
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider treating empty or blank token values the same as null in
validateToken, since an empty string will still pass the check but likely lead to confusing failures downstream. - You may want to include the token type (e.g., admin/master) in the error message directly, rather than only the property name, to make it clearer which credential is missing when multiple tokens are configured.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider treating empty or blank token values the same as null in `validateToken`, since an empty string will still pass the check but likely lead to confusing failures downstream.
- You may want to include the token type (e.g., admin/master) in the error message directly, rather than only the property name, to make it clearer which credential is missing when multiple tokens are configured.
## Individual Comments
### Comment 1
<location> `core/src/main/java/cz/xtf/core/helm/HelmBinaryManager.java:32-38` </location>
<code_context>
+ return getBinary(masterToken, OpenShiftConfig.namespace());
+ }
+
+ private String validateToken(String token, String propertyName) {
+ if (token == null) {
+ throw new IllegalStateException(
+ "Token is not configured. Please set '" + propertyName + "' in your properties file.");
+ }
+ return token;
}
</code_context>
<issue_to_address>
**suggestion:** Consider treating empty/blank tokens as invalid as well, not just null.
`validateToken` only rejects `null`. If `OpenShiftConfig.*Token()` can return an empty or whitespace-only string (e.g. property defined but with no value), that would still pass and likely fail later in a less clear way. Consider checking `token == null || token.isBlank()` (or `isEmpty()` if preferred) so misconfigurations are caught early with your explicit exception.
```suggestion
private String validateToken(String token, String propertyName) {
if (token == null || token.isBlank()) {
throw new IllegalStateException(
"Token is not configured. Please set '" + propertyName + "' in your properties file.");
}
return token;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kanovotn
force-pushed
the
missingOpenshiftToken
branch
from
6ec0bd4 to
c396a71
Compare
mnovak
approved these changes
Dec 3, 2025
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.
Description
This PR addresses issue #627 by adding validation in
HelmBinaryManagerto fail fast when the OpenShift admin token is not configured for Helm operations. This provides clearer error feedback instead of allowing operations to proceed and fail later with less informative error messages.Changes
HelmBinaryManager.javato verify OpenShift admin token is configured before proceeding with Helm operationscore/src/main/java/cz/xtf/core/helm/HelmBinaryManager.javaChecklist
Summary by Sourcery
Bug Fixes: