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

[bitnami/keycloak] add variables to support hostname v2 #70466

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

singhbaljit
Copy link
Contributor

@singhbaljit singhbaljit commented Jul 31, 2024

Description of the change

Use Hostname v2 options as v1 options are not used by default.

Benefits

Use updated configurations in alignment with upstream Keycloak project.

Possible drawbacks

KEYCLOAK_HOSTNAME variable is the same between v1 and v2 option; however, its usage is slightly different in v2. User may not realize the change and expect the same behavior as v1.

Applicable issues

Additional information

PR to use the options in the chart: bitnami/charts#28611.

@singhbaljit
Copy link
Contributor Author

I added support for two additional variables: KEYCLOAK_HOSTNAME_ADMIN and KEYCLOAK_HOSTNAME_STRICT with the same defaults as before. The chart will have to be updated to override the defaults based on ingress configs.

@singhbaljit singhbaljit changed the title [bitnami/keycloak] use hostname v2 options [bitnami/keycloak] use hostname v2 options with Keycloak v25 Jul 31, 2024
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Aug 1, 2024
@github-actions github-actions bot removed the triage Triage is needed label Aug 1, 2024
@github-actions github-actions bot removed the request for review from carrodher August 1, 2024 07:58
@github-actions github-actions bot requested a review from gongomgra August 1, 2024 07:58
Copy link
Member

@dgomezleon dgomezleon left a comment

Choose a reason for hiding this comment

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

Hi @singhbaljit ,

Please note that the logic is shared for both branches 24 and 25, so you should apply it for version 24 too. I don't see any risk in doing that since the default values did not change.
Could you please check it?

@singhbaljit
Copy link
Contributor Author

The variable names are the same, but their meaning is a little different in v24 (hostname v1) vs. v25 (hostname v2), namely hostname vs. hostname URL. The chart wants to specify the full URL, and it does so by using the v1 URL variables, bypassing these variables altogether. In other words, these variables won't be used in v24. They will be used in v25 after bitnami/charts#28611 is merged. But I can certainly add it if consistency is preferred.

@singhbaljit singhbaljit changed the title [bitnami/keycloak] use hostname v2 options with Keycloak v25 [bitnami/keycloak] add variables to support hostname v2 Aug 6, 2024
@singhbaljit
Copy link
Contributor Author

@dgomezleon I updated v24 scripts as well.

dgomezleon
dgomezleon previously approved these changes Aug 6, 2024
@dgomezleon
Copy link
Member

Thanks @singhbaljit

LGTM. Could you please check the conflicts?

@singhbaljit
Copy link
Contributor Author

singhbaljit commented Aug 6, 2024

The conflict is due to the fact that v24 is now removed 😄, see #70607 . So, I'll just undo the v24 changes.

Signed-off-by: Baljit Singh <baljit.singh@verizon.com>
@dgomezleon
Copy link
Member

The conflict is due to the fact that v24 is now removed 😄, see #70607 . So, I'll just undo the v24 changes.

Just today :). Sorry for the noise.

@dgomezleon dgomezleon merged commit f75effb into bitnami:main Aug 6, 2024
10 checks passed
@singhbaljit singhbaljit deleted the kc25 branch August 6, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keycloak solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/keycloak] Use Hostname v2 in Keycloak v25 images
4 participants