Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jakubschwan
Copy link
Collaborator

…scenarios using persistent test scenarioold test provider and change user impl will be removed in following commitClean framework and tests

@jakubschwan jakubschwan requested a review from Sgitario June 2, 2020 08:41
registerTrustedSecret(server);
if (!isCustomTrustedSecretRegistered(server)) {
registerTrustedSecret(server);
}
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

Copy link
Collaborator

@Sgitario Sgitario Jun 4, 2020

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?

…scenarios using persistent test scenarioold test provider and change user impl will be removed in following commitClean framework and tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants