Skip to content
This repository has been archived by the owner on Nov 16, 2022. It is now read-only.

Intly 3225 #39

Merged
merged 7 commits into from
Oct 4, 2019
Merged

Intly 3225 #39

merged 7 commits into from
Oct 4, 2019

Conversation

pb82
Copy link
Contributor

@pb82 pb82 commented Oct 1, 2019

JIRA ID

INTLY-3225

Additional Information

Creates an Ingress or Route if spec.externalAccess.enabled is set to True in the keycloak CR. A Route is only created on OpenShift. Also some refactoring.

Verification Steps

  1. Deploy keycloak from the provided example
  2. Make sure a Route gets created and that it works
  3. You should be able to use the route to login to the admin console using the credentials in the admin secret.

cmd/manager/main.go Outdated Show resolved Hide resolved
pkg/common/auto_detect.go Outdated Show resolved Hide resolved
pkg/model/constants.go Outdated Show resolved Hide resolved
pkg/common/auto_detect.go Show resolved Hide resolved
pkg/common/auto_detect.go Show resolved Hide resolved
pkg/common/auto_detect.go Show resolved Hide resolved
pkg/common/cluster_actions.go Outdated Show resolved Hide resolved
pkg/common/cluster_actions.go Outdated Show resolved Hide resolved
pkg/common/auto_detect.go Outdated Show resolved Hide resolved
Copy link
Contributor

@davidffrench davidffrench left a comment

Choose a reason for hiding this comment

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

@pb82 The changes are fantastic. I love the refactor of the autodetect, much better.

I have run it locally, I am not sure why but the reconcile loop is executing multiple times a second. I am not sure what is causing this. Would you be able to check if this is happening for you.

I am also not able to log into Keycloak with the admin username and password.

pkg/model/constants.go Outdated Show resolved Hide resolved
Copy link
Contributor

@slaskawi slaskawi left a comment

Choose a reason for hiding this comment

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

There are two major things that needs some thought:

  1. There's a clash between this PR and fix: Refactor admin secret #38
  2. There are lots of lint errors that probably it's worth to address

pkg/common/cluster_state.go Show resolved Hide resolved
pkg/common/cluster_state.go Outdated Show resolved Hide resolved
pkg/model/constants.go Outdated Show resolved Hide resolved
pkg/model/keycloak_service.go Outdated Show resolved Hide resolved
@davidffrench davidffrench mentioned this pull request Oct 2, 2019
4 tasks
pkg/model/constants.go Outdated Show resolved Hide resolved
Copy link
Contributor

@slaskawi slaskawi left a comment

Choose a reason for hiding this comment

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

Apart from one small comments, LGTM. Only Travis complains about the unit test errors.

pkg/model/constants.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 2, 2019

Coverage Status

Coverage decreased (-5.8%) to 30.308% when pulling a4891bc on INTLY-3225 into d7ddee3 on master.

slaskawi
slaskawi previously approved these changes Oct 4, 2019
@@ -130,6 +130,8 @@ github.com/mitchellh/go-homedir
github.com/modern-go/concurrent
# github.com/modern-go/reflect2 v1.0.1
github.com/modern-go/reflect2
# github.com/openshift/api v3.9.0+incompatible
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this intentional? If so, can we just remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed because it contains the Route type

@slaskawi slaskawi merged commit ae9e507 into master Oct 4, 2019
@keycloak-bot keycloak-bot deleted the INTLY-3225 branch July 22, 2020 08:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants