-
Notifications
You must be signed in to change notification settings - Fork 129
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
Identity-operate integration #268
Conversation
Introduce new methods, which can be reused (e.g. seeionBasedLogin). Remove unnecessary parameters, return values etc.
Per default secret for operate-identity is auto-generated which makes it hard to test with golden files. We ignore this secret line in the goldenfiles, and test is separately.
|
||
> kubectl port-forward svc/{{ .Release.Name }}-identity 8080:80 | ||
> kubectl port-forward svc/{{ .Release.Name }}-keycl 18080:80 | ||
> kubectl port-forward svc/{{ include "common.names.dependency.fullname" (dict "chartName" "keycloak" "chartValues" .Values.identity.keycloak "context" $) | trunc 20 | trimSuffix "-" }} 18080:80 |
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.
Keycloak has the weird property to cap the names at 20 chars, the reason is that wildfly only supports a certain amount of characters 🤷 😆
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.
🚀
@dlavrenuek - I didn't really check if it's correctly configured or working properly, just that it starts. I think you will be better placed to check that it works as expected.
Other notes:
- ❓ The version is 8.0.0-rc1, I guess we'll bump this to 8.0.0 soon? Everything should be available now.
- ❓ There are 4 configuration settings for the integration, but only two of them are tested in the deployment test, and one in the secret test - any reason for not testing the last one about the root URL? Or maybe I missed it.
Anyway, no blockers that I could see
Thanks for your review @npepinpe 👍 Regarding your questions:
I will do it after I merged all PR's, otherwise we have to many conflicts.
The others are tested via golden files. In the template tests (in deployment test) we only test conditional properties. I will merge it after @dlavrenuek gives us the go :) |
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.
Some small comments but not a deal breaker from my side
Thanks for your reviews I will merge this. I think we can continuously improve if we see anything. |
Integrates Identity with Operate.
New notes:
@dlavrenuek maybe you can shortly check whether the config is like we discuss ( I would expect, just to be sure)
@npepinpe please do a normal review :)