-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Allow host:port syntax for --debug in kc.sh #39924
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
Conversation
Loosen the --debug argument parsing in kc.sh to accept full host:port addresses (e.g. 0.0.0.0:8787 or *:8787) in addition to plain port numbers. This enables JDWP to bind to all container interfaces when running Keycloak in Docker, without requiring manual JAVA_OPTS overrides or script edits. Closes keycloak#38924 Signed-off-by: mircea.talu <talumircea13@gmail.com>
Thank you for the pr @Mirciulica15. I believe this looks good. Can you address:
|
Signed-off-by: mircea.talu <talumircea13@gmail.com>
@shawkins Added support for IPv6, I will later look for a good place to put this in the docs. As for the kc.bat script, I'd like to test my changes before committing. kc.sh was relatively easy to test by volume mounting in the container via podman run -v "$(pwd)/kc.sh:/opt/keycloak/bin/kc.sh:ro" -p 8080:8080 -p 8087:8087 localhost/keycloak-fix-address:local start-dev --debug [::0]:8087 Do you have any advice for testing kc.bat? Thanks! |
I've locally used wine, but it doesn't always faithfully reproduce. cc @Pepo48 do you have any advise on windows testing? We can always try to have someone on the keycloak team round out the pr for the kc.bat handling once we're set on the kc.sh logic. |
Signed-off-by: mircea.talu <talumircea13@gmail.com>
Signed-off-by: mircea.talu <talumircea13@gmail.com>
…sses Signed-off-by: mircea.talu <talumircea13@gmail.com>
Signed-off-by: mircea.talu <talumircea13@gmail.com>
Thank you for the update. Hopefully updating kc.bat won't be so bad now. |
@Mirciulica15 cc:@shawkins with regards to testing kc.bat I'm afraid that I can only offer the obvious options - windows bare metal, VM or eventually a container (for non UI test cases). We typically use mix of all - win-server VMs in our internal pipelines and containers in Github CI for the testsuite execution; and I can certainly test the PR even on my physical machine, as Steve already mentioned. |
@Pepo48 I managed to get a hold of a Windows machine for testing, but I am facing several issues while trying to implement the regexes in the .bat script. For example, a one-liner like the one below works perfectly when I test it in my terminal, but always fails within the kc.bat script: echo 127.0.0.1 | findstr /R /X "^[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*$" >nul && echo validIPv4 || echo notIPv4 I've tried changing the file encoding according to certain threads I have found, but the issue persists. Have you encountered this issue previously? EDIT: The kc.bat script appears to become increasingly long and complex, especially with these latest changes. Echoing @shawkins's previous concerns about the complexity in the .bat script, do you think that migrating it to a more modern/maintainable alternative could be considered? |
@Mirciulica15 I guess that by a modern alternative you mean PowerShell, right? It's not on our radar right now, since there hasn't been a demand for it. The main concern is about maintainability - we don't want to maintain two separate scripts for one platform. On top of that, there might be still some users, who are running their instances within environments, where PowerShell is not/can't be installed. In other words, guaranteed availability + non-existing demand for a change are the main reasons, why the bat script is still used. Regardless, feel free to open a discussion if you consider it to be important. It may raise some attention from the community and get some serious consideration. |
@Mirciulica15 regarding the regex, my initial guess it that this may be caused by Windows findstr - it has known issues with the end anchor $ when used with piped input. A simple demonstration of this limitation would be: Unfortunately, in order to validate that nothing comes after the expected pattern I haven't found any other way than just a string manipulation:
( Let me know if it solves something for you. Thanks! |
Approved the workflow run for the kc.sh change to validate it's working as expected. Hopefully kc.bat won't be too burdensome to update. |
@Mirciulica15 Please fetch the latest changes from the main branch, as it should resolve the Windows issues in the CI. Thanks! EDIT: Ahh, ok... I was probably able to do that :) |
Signed-off-by: mircea.talu <talumircea13@gmail.com>
Signed-off-by: mircea.talu <talumircea13@gmail.com>
Sorry for the delay guys, I've been busy with my final exams these past few weeks. Thanks for the tips @Pepo48, I worked on the kc.bat script a bit today and implemented the logic, trying to stay as close to kc.sh as possible. I tried the |
Signed-off-by: mircea.talu <talumircea13@gmail.com>
@shawkins I have added the new support for the |
That looks good to me. Thank you! |
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.
LGTM. @Mirciulica15 Thank you for the contribution!
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.
@Pepo48 Can you please check the kc.bat
changes?
@Mirciulica15 thanks for the contribution! I just rechecked the Windows part - syntactically, everything looks good. Functionality-wise, I was able to run Keycloak in the debug mode with a specific debug address, attach the debugger, and hit the expected breakpoint, so the behavior is as intended. Therefore, I’m approving the PR. Great job! |
As the person who was originally bitten by the issue and reported it, thank you for working on this @Mirciulica15! |
Congratulations, you did a great job with feadback. |
Loosen the --debug argument parsing in kc.sh to accept full host:port addresses (e.g. 0.0.0.0:8787 or *:8787) in addition to plain port numbers. This enables JDWP to bind to all container interfaces when running Keycloak in Docker, without requiring manual JAVA_OPTS overrides or script edits.
Closes #38924