Skip to content
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

upgrade to Keycloakify 11 #3272

Merged
merged 52 commits into from
Nov 26, 2024
Merged

upgrade to Keycloakify 11 #3272

merged 52 commits into from
Nov 26, 2024

Conversation

fhennig
Copy link
Contributor

@fhennig fhennig commented Nov 21, 2024

resolves #3264

preview URL: http://keycloakify-upgrade-try-a.loculus.org

Summary

The old keycloakify 9 theme was replaced by a rewrite based on keycloakify 11 (this was the recommended upgrade path). All features of the old theme were copied over. Also, the new theme uses an env var to set the project name, replacing the old email texts that simply omitted the realm name.

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by an appropriate test.

@fhennig fhennig mentioned this pull request Nov 21, 2024
2 tasks
@fhennig
Copy link
Contributor Author

fhennig commented Nov 21, 2024

In 5e0c509 I'm applying the fix from @theosanderson from this PR: #3271

Weird that this just happens 'out-of-the-box' with the starter. Thanks again to @theosanderson for finding this!

@fhennig fhennig added the preview Triggers a deployment to argocd label Nov 21, 2024
@fhennig fhennig self-assigned this Nov 21, 2024
@fhennig
Copy link
Contributor Author

fhennig commented Nov 21, 2024

It's working in the preview! 🥳

@corneliusroemer
Copy link
Contributor

After I found the root cause of the CI failure (wrong casing of kcContext that didn't show on macOS due to file system casing insensitivity) I wanted to report the issue to keycloakify but it turns out that the casing isn't in the starter: https://github.com/keycloakify/keycloakify-starter/blob/main/src/login/KcContext.ts

@fhennig how did you create the starter? Did you maybe not copy it over manually somehow rather than a complete removal over folder and replacement with current starter?

As a result, the fix is easier than Theo's which just happened to fix casing as well but the types extension was unnecessary.

@fhennig
Copy link
Contributor Author

fhennig commented Nov 21, 2024

What I did before was clone into keycloakify-starter, mv keycloakify keycloakify9 and mv keycloakify-starter keycloakify. I have no idea why the file was lowercase :O Well spotted! I thought I actually checked that as well, oh well.

I pushed the change now! It's actually good, because there is kc.gen.tsx which gets regenerated if the theme name changes and it contains a reference to KcContext, so it's nice that the file will be generated with the correct name.

Wow I feel stupid now 🤦 😅

@theosanderson
Copy link
Member

The file was named wrongly on main by me in the old version – now that it occurs to me I think I have dim memories of that. Some original eslint thing was giving me warnings and I briefly tried to fix them and then gave up or something

@corneliusroemer
Copy link
Contributor

You weren't stupid at all @fhennig - you actually did everything right and macOS (and git) played a trick on us! This is just something we need to remember can happen 🤷

@corneliusroemer
Copy link
Contributor

I've now added the keycloak update to 26, might as well test it with that.

@corneliusroemer
Copy link
Contributor

It's super confusing that version 26 is more like version 21 than 22-25 here: keycloakify/keycloakify#731

@fhennig
Copy link
Contributor Author

fhennig commented Nov 25, 2024

Still WIP:

  • The "Update user profile" page that you get to after signup with ORCID doesn't have the tickbox yet.
  • use the env var again for the Text for the checkbox
  • optional: add an env var where a "realm name" can be set, so we can display something that makes it clear we're still on loculus/pathoplexus

@fhennig
Copy link
Contributor Author

fhennig commented Nov 25, 2024

I think some docs might be missing still, but I think the new theme is feature-complete now! (oh and I didn't delete the kc9 theme yet)

@fhennig fhennig force-pushed the keycloakify-upgrade-try-again branch from 0b68684 to 6b073c5 Compare November 26, 2024 09:59
@fhennig
Copy link
Contributor Author

fhennig commented Nov 26, 2024

I tried registering with KC in the preview and get an email, but I think no email arrived.

@fhennig fhennig marked this pull request as ready for review November 26, 2024 10:01
@theosanderson
Copy link
Member

I tried registering with KC in the preview and get an email, but I think no email arrived.

I just tried and seemed to see the same. I tried an unrelated preview (https://profile-get-seqs.loculus.org/) and registration was fine, so that seems like a problem

@fhennig
Copy link
Contributor Author

fhennig commented Nov 26, 2024

Alright so I just removed the e-mail theme and then I got an email when registering. (EDIT: I have removed the commit now that did this)

@fhennig
Copy link
Contributor Author

fhennig commented Nov 26, 2024

Maybe I shouldn't have deleted the theme.properties 😆 I'll see if that maybe was the issue.

When initializing the theme it says:

The src/email directory have been created.
You can delete any file you don't modify.

But in the docs it says that the theme.properties are needed to specify the base theme:

You can remove all the template and resource file you aren't going to customize (it will fallback to the default email theme as long as you keep a theme.properties with parent=base). [source]

@fhennig fhennig force-pushed the keycloakify-upgrade-try-again branch from 59756e3 to a7467af Compare November 26, 2024 12:50
@theosanderson
Copy link
Member

I changed something (elsewhere) so the ORCiD flows now work - and they lgtm

@fhennig
Copy link
Contributor Author

fhennig commented Nov 26, 2024

Found what I did wrong, I'll have a fix in a bit.

@fhennig
Copy link
Contributor Author

fhennig commented Nov 26, 2024

I got an email now!

Copy link
Member

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

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

Thanks this looks nice! It's the sort of thing that is pretty hard to review confidently for code, but I've tested the functionality in various ways and things seem good!

@fhennig fhennig mentioned this pull request Nov 26, 2024
2 tasks
@fhennig
Copy link
Contributor Author

fhennig commented Nov 26, 2024

great! I have also updated the kc26 branch here: #3299 and it also looked good there! 🥳

@fhennig fhennig merged commit 44c0534 into main Nov 26, 2024
16 checks passed
@fhennig fhennig deleted the keycloakify-upgrade-try-again branch November 26, 2024 13:51
@corneliusroemer
Copy link
Contributor

I also tested stuff yesterday, got an email and looked good, with my Keycloak 26 changes on top (before they were added).

Deploying KC26 together with keycloakify 11 is a bit of a risk as we can't roll back keycloakify anymore. So prudently we'd deploy keycloakify first but considering it's unlikely we need to roll back keycloakify 11 this is ok.

Just pointing out the tradeoff.

@@ -40,7 +40,5 @@ jobs:
run: yarn install --immutable
- name: Build
run: yarn build
- name: Build storybook
run: yarn build-storybook
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use storybook we should test it in CI so we can upgrade without surprises. I.e. don't revert this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

build-storybook doesn't exist anymore, and there isn't a replacement AFAIK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Triggers a deployment to argocd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Keycloakify 11
3 participants