-
Notifications
You must be signed in to change notification settings - Fork 19
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
RHPAM-2762 & RHPAM-2777 add tests to verify issues fixImplement test … #633
base: main
Are you sure you want to change the base?
Conversation
...oud/openshift/operator/scenario/builder/WorkbenchKieServerPersistentScenarioBuilderImpl.java
Show resolved
Hide resolved
registerTrustedSecret(server); | ||
if (!isCustomTrustedSecretRegistered(server)) { | ||
registerTrustedSecret(server); | ||
} |
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.
Why do we need to check whether the ssl is configured? The values will remain the same, so it should not matter if we set the environment variables again and they won't change in the second execution, right?
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.
Yes, the value remain same but once when ssl is configured, I think there is no need to creating it again. If there is existing configuration, we should use it. It's also closer to manual update, when is KieApp updated manually we do not created the custom secret again.
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.
Ok, I see the problem. The "env" field is a list (not a set), if we don't check this, we'll be adding the same value several times.
Maybe in order to avoid having the same hard coded values in difference places (example: "HTTPS_NAME" or "HTTPS_PASSWORD"), what about adding a transient method in the Server.java to add the env if it does not exist?
@Transient
public void addEnvIfNotSet(String name, String value) {
if (Stream.of(getEnv()).map(Env::getName).noneMatch(envName -> StringUtils.equals(envName, name))) {
addEnv(new Env(name, value));
}
}
Or adding this new method in OpenShiftOperatorScenario sending the Server instance:
public void addEnvIfNotSet(Server server, String name, String value) {
if (Stream.of(server.getEnv()).map(Env::getName).noneMatch(envName -> StringUtils.equals(envName, name))) {
server.addEnv(new Env(name, value));
}
}
Then, it would be only about to replace the existing "addEnv" method to "addEnvIfNotSet" in OpenShiftOperatorScenario. We would not need to worry about having new methods to check existing envs or deal with duplicated hardcoded values.
What do you think?
...java/org/kie/cloud/openshift/operator/scenario/WorkbenchKieServerPersistentScenarioImpl.java
Show resolved
Hide resolved
…scenarios using persistent test scenarioold test provider and change user impl will be removed in following commitClean framework and tests
044340a
to
d81d089
Compare
…scenarios using persistent test scenarioold test provider and change user impl will be removed in following commitClean framework and tests