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

Add support for more http-related configs #115

Merged
merged 5 commits into from
Nov 13, 2023

Conversation

gionn
Copy link
Contributor

@gionn gionn commented Oct 9, 2023

  • keycloak_quarkus_http_relative_path var now populate http-relative-path config [breaking change]
  • http-relative-path defaults to / [breaking change]
  • enable configuration of hostname-url and hostname-admin-url

Fix #114

@guidograzioli guidograzioli added the breaking_changes MUST include changes that break existing playbooks label Oct 16, 2023
@gionn gionn marked this pull request as ready for review October 16, 2023 15:07
@gionn gionn marked this pull request as draft October 25, 2023 08:23
@guidograzioli
Copy link
Member

I am merging the proxied deploy molecule test (disabled in CI); can you please rebase on top of HEAD, and enable it?

@gionn
Copy link
Contributor Author

gionn commented Oct 27, 2023

I am merging the proxied deploy molecule test (disabled in CI); can you please rebase on top of HEAD, and enable it?

it seems not yet merged, will wait for it.

FYI I've used these changes in my downstream PR and solved the issues I was facing

@guidograzioli
Copy link
Member

I am merging the proxied deploy molecule test (disabled in CI); can you please rebase on top of HEAD, and enable it?

it seems not yet merged, will wait for it.

FYI I've used these changes in my downstream PR and solved the issues I was facing

You right, I've been stolen away from this PR too long. I have merged the molecule test (disable), can you please rebase, and commit a change here to enable it? "https_revproxy" in the list here

* keycloak_quarkus_http_relative_path var now populate http-relative-path config [breaking change]
* http-relative-path defaults to / [breaking change]
* enable configuration of hostname-url and hostname-admin-url
@gionn
Copy link
Contributor Author

gionn commented Nov 7, 2023

https_revproxy failing, an expected environment variabile missing?

  fatal: [proxy -> localhost]: FAILED! => {"changed": false, "msg": "Client ID not specified and unable to determine Client ID from 'REDHAT_PRODUCT_DOWNLOAD_CLIENT_ID' environment variable."}

https://github.com/ansible-middleware/keycloak/actions/runs/6782416019/job/18434593913?pr=115#step:5:238

@guidograzioli
Copy link
Member

guidograzioli commented Nov 7, 2023

It fails because the PR from your fork cannot read the secrets, let me verify it

It also fails because the molecule tests sets:

keycloak_quarkus_http_relative_path: /

this turns into 3 slashes in the health endpoint in vars:

health_url: "http://{{ keycloak_quarkus_host }}:{{ keycloak_quarkus_http_port }}/{{ keycloak_quarkus_http_relative_path }}{{ '/' if keycloak_quarkus_http_relative_path else '' }}realms/master/.well-known/openid-configuration"

which confuses keycloak (it returns 404 resource not found).

I'll let you decide if you prefer to have keycloak_quarkus_http_relative_path default to '/' (and fix the health url concatenation), or to keep empty string and only fix the health_url

Otherwise, the changeset LGTM, and we can merge :)

@gionn
Copy link
Contributor Author

gionn commented Nov 13, 2023

I preferred to switch to / as default to be consistent with upstream docs, let me know how a new test run goes.

Copy link
Member

@guidograzioli guidograzioli left a comment

Choose a reason for hiding this comment

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

LGTM

@guidograzioli guidograzioli marked this pull request as ready for review November 13, 2023 10:36
@guidograzioli guidograzioli merged commit 24787e4 into ansible-middleware:main Nov 13, 2023
6 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_changes MUST include changes that break existing playbooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Troubles with console access behind a proxy when setting keycloak_quarkus_http_relative_path
2 participants