Skip to content

Add keycloak support #6

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add keycloak support #6

wants to merge 4 commits into from

Conversation

fydai
Copy link
Contributor

@fydai fydai commented Feb 17, 2020

No description provided.

@@ -1,21 +1,15 @@
FROM docker.ocf.berkeley.edu/theocf/debian:buster
FROM debian:buster
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not using OCF Debian? AIUI the main benefit is that it's configured to use our mirrors. If you want to keep things lightweight, you could look into using Alpine instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also includes a bunch of OCF specific stuff, specifically a bunch of ldap configuration and kerberos configuration. If a service doesn't depend on it, I would prefer to not use the OCF docker image for determinism's sake. I'm indifferent to the actual OS used, but seeing as it's already configured on debian, I don't see a reason to move to alpine or something.

Regarding mirrors usage, this thing will be rebuilt once every blue moon, so I don't think it there's a pressing need to use OCF mirrors. If you think that's a good idea however, I can spend ~10 minutes putting something in sources.list.

Copy link
Member

Choose a reason for hiding this comment

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

We get some things like the correct timezone used, backports enabled, etc. that I think would be useful to have too. It also makes getting security updates and getting changes into all services much easier for us.

Does having the ldap config and all that extra matter much?

Copy link
Member

@jvperrin jvperrin left a comment

Choose a reason for hiding this comment

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

How have you tested this so far?

@@ -1,5 +1,3 @@
servicePipeline(
upstreamProjects: ['dockers/master'],
Copy link
Member

Choose a reason for hiding this comment

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

This is here so we get security updates, so I think it'd be better if it was left in place

Copy link
Member

@jvperrin jvperrin left a comment

Choose a reason for hiding this comment

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

I really like what you've done here so far, it would remove the basic auth and make logging more standard (with loki and all that). I'd be happy to approve it but do still think using our standard docker images and upstream build would be better, even if there is some extra overhead with it. Thoughts?

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.

3 participants