-
Notifications
You must be signed in to change notification settings - Fork 34
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
chore: upgrade to latest Keycloak chart #2399
Conversation
You can access the deployment of this PR at https://ci-renku-2399.dev.renku.ch |
did you test to see what happens when you upgrade from the current version to this new one? Any db migrations to be aware of? |
No, I didn't do any tests, I thought it was a trivial upgrade, but as the deployment shows it isn't really. I'll close this PR to avoid confusion. I believe @ableuler will pick this up when his pipeline clears up. |
What was the problem with the deployment? |
It did deploy, actually: https://github.com/SwissDataScienceCenter/renku/runs/4725939662?check_suite_focus=true
😆 hope dies last |
it deploys, but the last time I've checked all the tests were failing to authenticate... |
002406a
to
d08cc8e
Compare
a98f509
to
ccdcb2f
Compare
While the current version of Keycloak is not explicitly affected by Log4shell, it is however safer to ship a more recent version.
do not fail if the secret is not set, as is the case for public clients. It seems that in current version of Keycloak the server doesn't return a secret for public clients (makes sense) but in older versions it did (doesn't make sense).
This reverts commit e20f6eb.
b4b9eaf
to
4c535c9
Compare
@aledegano @ableuler I tested this in rok.dev.renku.ch by first installing the current |
Wow, great, thanks for picking this up! |
I'm still considered the author, so I cannot approve it 🤣 |
@@ -6,7 +6,7 @@ name = "pypi" | |||
[packages] | |||
"urllib3" = ">=1.24.2" | |||
"jinja2" = ">=2.10.1" | |||
chartpress = "==0.7.0" | |||
chartpress = "==1.0.0" |
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.
Wasn't there a reason for pinning this to 0.7.0 @aledegano ?
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.
CI is using 1.0... I think that was part of the reason for the weird build discrepancy we were looking at last week @ableuler
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.
IIRC the 0.7.0 was the latest version that still supported having the chartpress file in a different folder than that it expected.
However this was ~1 year ago and things might have changed(?)
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.
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.
hmm when is this a problem?
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.
afaict all of our chart building actions work fine with this change. But maybe I'm not hitting all the places that could break?
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.
🤷
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 just ran chartpress
locally from the base directory in this repo and it build all the images fine. So I'm not sure that "feature" of 1.0 matters here?
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 confirm that works for me too, I don't know what to say...
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.
alright then, let's merge this and remind ourselves that this might be a problem if pipelines suddenly started to fail :)
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.
Thanks @rokroskar for taking care of this!
While the current version of Keycloak is not explicitly affected by Log4shell, it is however safer to ship a more recent version.
/deploy