-
Notifications
You must be signed in to change notification settings - Fork 170
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
[THREESCALE-1289] Fix error when loading a config with only some services using OIDC #940
Conversation
f1e6277
to
772d638
Compare
t/configuration-loading-with-oidc.t
Outdated
@@ -0,0 +1,232 @@ | |||
use lib 't'; | |||
use Test::APIcast 'no_plan'; |
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.
Can't it be a blackbox test?
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.
I based these tests on the ones we already have that test the configuration loading.
I'm not sure if a Blackbox test would work in this case because we'd need to define all those locations to get the configuration and there wouldn't be a "---configuration" directive.
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.
It would work with lazy loading the configuration.
My point is that Blackbox is 100% real APIcast. This is somewhat similar to APIcast but not really. And we really should aim to not add more of these tests as they are not compatible with the liquid templating of the nginx config.
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.
Fair enough. I changed it.
7052447
to
bd60b9c
Compare
I fixed the codeclimate checks and also adapted a couple of tests of the management endpoint (bd60b9c) |
This is now tested in "configuration-loading-with-oidc.t". In the tests of this file, we are mocking the "remote_v2" loader, we cannot properly test loading on boot. That's the reason why the test deleted in this commit does not add any value over the new ones added in "configuration-loading-with-oidc.t".
bd60b9c
to
8941805
Compare
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.
That is excellent 👍 🥇
This PR fixes properly the bug that we tried to fix in #893
The bug was still there because the integration tests that load the configuration on boot mock the loader v2 module. In the previous PR, we fixed the bug in the remote loader v2. Unfortunately, the same bug was present also in the OIDC loader module and we didn't have tests for that.
In this PR, I've added integration tests that properly test the whole loading flow including the OIDC loader module.
Ref: https://issues.jboss.org/browse/THREESCALE-1289