Skip to content

Improvements on Operator Integration Tests to make sure that Config set in CR takes precedence #16639

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.

Sign up for GitHub

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

Jump to bottom
Merged
vmuzikar merged 1 commit into from
Feb 23, 2023
Merged

Improvements on Operator Integration Tests to make sure that Config set in CR takes precedence #16639

vmuzikar merged 1 commit into from
Feb 23, 2023

Conversation

@andre-nascimento6791
Copy link
Contributor

@andre-nascimento6791 andre-nascimento6791 commented Jan 26, 2023

In order to make sure that the Config set in CR takes Precedence over the default values and hence none Environment Variable gets duplicated in Deployment, then just some improvements on Operator Integration Tests were implemented.

Closes #13114

@andre-nascimento6791 andre-nascimento6791 force-pushed the issue-13114 branch 5 times, most recently from 3466964 to b960274 Compare January 30, 2023 17:43
@andre-nascimento6791 andre-nascimento6791 requested a review from a team as a code owner January 30, 2023 17:43
@ghost ghost added the flaky-test label Jan 31, 2023
@ghost ghost mentioned this pull request Jan 31, 2023
Closed
@andre-nascimento6791 andre-nascimento6791 force-pushed the issue-13114 branch 3 times, most recently from ab885f9 to 5dac86f Compare February 2, 2023 09:37
vmuzikar
vmuzikar requested changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andre-nascimento-rh Thank you for this PR. Added a few comments.

@ghost ghost mentioned this pull request Feb 16, 2023
Closed
@andre-nascimento6791 andre-nascimento6791 force-pushed the issue-13114 branch 4 times, most recently from 0cdc4ae to 4e9d50b Compare February 21, 2023 14:56
ghost
ghost reviewed Feb 21, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@ghost
Copy link

ghost commented Feb 21, 2023

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.ui.account2.WelcomeScreenTest#applicationsTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Expected ApplicationsPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/applications&state=f52e54dc-1aa9-484b-810e-b8d829a15632&session_state=a680b2d7-940c-44b9-a9fd-aaa0c1efb356&code=3c4126ab-33f5-485a-ad32-7a9af6e94e5b.a680b2d7-940c-44b9-a9fd-aaa0c1efb356.b073e9db-cc97-47d5-b2b0-8adc25dafeb0)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at jdk.internal.reflect.GeneratedMethodAccessor106.invoke(Unknown Source)
...

Report flaky test

vmuzikar
vmuzikar requested changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andre-nascimento-rh Thanks for the update, added two more comments.

@andre-nascimento6791
Copy link
Contributor Author

andre-nascimento6791 commented Feb 22, 2023

Due to the fact that the Operator does not support overriding or removing Env Vars throughout PodTemplate, makes no sense to have such test method. Therefore, I'm removing all this new method from PodTemplateTest class.

Thanks.

vmuzikar
vmuzikar approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@vmuzikar vmuzikar enabled auto-merge (rebase) February 23, 2023 07:30
@vmuzikar vmuzikar merged commit aa42248 into keycloak:main Feb 23, 2023
@andre-nascimento6791 andre-nascimento6791 deleted the issue-13114 branch February 23, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vmuzikar vmuzikar vmuzikar approved these changes

@abstractj abstractj Awaiting requested review from abstractj

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reencrypt proxy ignored with new operator

3 participants

@andre-nascimento6791 @abstractj @vmuzikar