Repository navigation
Conversation
|
Thank you for this PR. For contributions, we ask to sign-off commits, see CONTRIBUTING.md for details. Please update your PR. |
af9142b to
8176329
Compare
Closes keycloak#45805 Signed-off-by: areevesat <andrew.reeves@angeltrax.com>
32d51ad to
2f77fa5
Compare
|
I have updated the PR to include the signoff in the commit message, as well as to include the note of which issue it closes in the message. Please let me know if anything else is required. |
There was a problem hiding this comment.
Hi @areevesat, first of all, thank you for the contribution and for taking the time to improve the documentation.
I think the intent behind this change makes sense, but I’m not sure the message itself is quite right. Keycloak is a multi-platform project, and our goal is not to encourage users to rely on tools like Git Bash, which are effectively compatibility layers on top of Windows (MSYS2). Instead, we explicitly aim to ensure that everything can be done using native Windows shells such as PowerShell or CMD, just as it can be done on Unix-like systems with Bash.
That said, I completely agree that there is significant room for improvement across our READMEs and documentation when it comes to cross-platform clarity. This is an area where the project could definitely do better.
Specifically for the files touched by this PR, the vast majority of commands already work on Windows with only minor, well-known adaptations, for example:
export VAR=value→set VAR=valuein CMD or$env:VAR="value"in PowerShellVAR=value command(inline env vars) → set the variable first, then run the command
These are fairly standard cross-platform differences, and in general developers are already used to them.
The broader question, then, is whether we want to adopt a more systematic approach to this across the documentation. For example, Quarkus uses mechanisms like AsciiDoc tab switching to present all sorts of variants where it makes sense. If we were to do something like that for OS-specific variants, it would probably be more impactful to apply it consistently in areas such as containers.adoc, reverse-proxy.adoc, and similar guides, rather than addressing individual cases in isolation.
We’ve discussed this internally before, but since there hasn’t been strong demand or many complaints from the community, we decided to put it on hold for now. That said, if you’d like to repurpose this PR or propose a more general approach, we’d be happy to discuss it further.
Thanks again for the contribution and for engaging with the project.
|
Thank you for getting back to me. My primary motivation to create this issue and PR in the first place was the frustration and hours of troubleshooting I ran into while trying to set up my environment for initial debugging. See my discussion #44921 (I still am not able to actually debug Keycloak locally, likely due to unknown setup issues). At least one maven command did not function in PowerShell, and only worked once I swapped to Git Bash. It does seem that there is still some cross-platform compatibility needed there. Certainly I could modify the messaging to not mention a specific tool and instead just reference a class of tools or general approach. My goal was simply to help the next person avoid the pitfall of running the commands as-is in an environment where they won't work. I think any solution that assists new developers in getting started with the project would be of benefit to onboarding. In my opinion, the instructions, especially the getting started instructions, ought to work out of the box without relying on the developer to already know where they need to deviate from the instructions. I think it is likely that you don't receive complaints because most people who have d 8000 ifficulty would just give up rather than raise a fuss. That said, I do not think I have the expertise or familiarity with the project to make a broader solution. I could repurpose the original issue, but I think you and the rest of the existing community would be better able to make any larger-scale changes in a manner consistent with this project's style and principles. |
|
@areevesat is this what you've tried to achieve specifically on Windows? I think you can clearly grasp everything I did from the screenshot, but here's a recap, just in case:
As you can see the execution stopped exactly at the breakpoint and watched variables are initiated. |
|
@areevesat alternatively, if you want to run everything from the IDE, you can follow what @mabartos already advised in #44921 (comment) - run the Just to be completely sure, I tested this on Windows as well - everything works the same as shown in the screenshot above. |
|
When I attempt ./kc.bat --debug start-dev, it hangs indefinitely right after "Listening for transport dt_socket at address: 8787". Running ./kc.sh --debug start-dev in Git Bash appears to work. When I followed the instructions mabartos mentioned, I was unable to locate the XML settings he referenced. And ran into the problems I mentioned in a comment in that discussion, with missing CSS and no database to persist data between runs. |
@areevesat do you use Because of that, you either need to stop the debugger and attach it again, or simply not set The XML settings are just a reference to show roughly what needs to be configured, especially if you’re trying to run this from an IDE other than IntelliJ. You likely can’t use them directly in the Eclipse IDE you mentioned, but I assume the steps to run a class in debug mode are very similar. |
|
Thank you for your patience and working with me on this. I was able to get the debugging process working by running the quarkus dist from the command line, then attaching the IntelliJ IDEA debugger to. Your instructions were very helpful, but they don't seem to map to the instructions in https://github.com/keycloak/keycloak/blob/main/quarkus/README.md. I'd be happy to update my original issue (#45805) with more ideas about what documentation could be improved. Thank you again for helping me with the debugging steps. |
@areevesat bear in mind that right next to In practice, it doesn’t really matter which form of the server you attach the debugger to - whether you built or downloaded the distribution, or whether you use the embedded server or |
|
@Pepo48 |
Merging the two documents (`README.md` and `CONTRIBUTING.md`) should provide information like "How to debug Keycloak in Quarkus DEV mode?" or "How to include Keycloak as extension to my existing Quarkus app?" in the one document users read whether they intend to contribute to Keycloak project or not. The new `README.md` also contain infor 8000 mation specific to contributing, which is fine as the document is not part of the official Keycloak documentation. The `README.md` is intended as a guide for developers. This change is close to what the issue reporter suggested keycloak#45806 (comment), and is how we agreed to solve the issue when discussing this in Keycloak cloud native team. Closes: keycloak#45805 Supersedes: keycloak#45806 Changes: - `Before you start` section newly mentions `config-api` sub-module and Quarkus word is capitalized - `Activating the Module from the root directory` section is dropped as it is not true: - Quarkus module is not enabled conditionally - Quarkus module compilation with JDK 11 fails as Quarkus baseline is JDK 17 and JDK version is enforced in Keycloak parent by `enforce-java-version` - `Building the Keycloak Quarkus distribution` section - capitalized Quarkus word - `Running from the command line` section contained link leading to unknown discussion https://github.com/keycloak/keycloak/discussions/categories/keycloak-x-quarkus-distribution, I replaced it with general GH discussion link. I could not verify the reload statement due to the issue described below, but judging by this discussion keycloak#14351 explanation about reload limitations is still valid - `Running tests` section used `Keycloaks` which I think was a typo, so I fixed it. And capitalized Quarkus word - `Running tests from your IDE` I changed statement about the `@DistributionTest` test being a blackbox, I think the build may happen in a JVM if it is container environment, otherwise it is just a different JVM process - `Running container-based tests` capitalized word `Docker`. I also dropped `The container based tests are using Testcontainers to spin up the container image and can be considered tech preview.` as 3 years seem long enough for container-based tests to mature - `Running database tests`, fixed few typos - `Updating Expectations` fixed command example and test class rendering - `CONTRIBUTING.md` is merged into `README.md` in following manner: - `Keycloak Quarkus Extension` section, renamed `Resteasy` to `Quarkus REST` and missing recorder method `configSessionFactory` with `configSessionFactory` - dropped `Running the server in development mode` section as it was dealing with the same topic as the `Running from the command line` `README.md` section. however I migrated few sentences - dropped `Running tests` as it deals with the same topic as `Running tests` section of the README and it contains everything the dropped section does, just in a different words - `Before contributing the changes` section content replaced the README `Contributing` section as it provides a better approach - `Documentation` fixed link to the server documentation guides What is not handled here: - `Running from the command line` section suggests command `../mvnw -f server/pom.xml compile quarkus:dev -Dkc.config.built=true -Dquarkus.args="start-dev"`, however running this command on my workstation leads to startup failure due to missing class `java.lang.ClassNotFoundException: org.osgi.framework.FrameworkUtil` - former `CONTRIBUTING.md` keeps stating that the bytecode is recorder during the runtime Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
Merging the two documents (`README.md` and `CONTRIBUTING.md`) should provide information like "How to debug Keycloak in Quarkus DEV mode?" or "How to include Keycloak as extension to my existing Quarkus app?" in the one document users read whether they intend to contribute to Keycloak project or not. The new `README.md` also contain information specific to contributing, which is fine as the document is not part of the official Keycloak documentation. The `README.md` is intended as a guide for developers. This change is close to what the issue reporter suggested keycloak#45806 (comment), and is how we agreed to solve the issue when discussing this in Keycloak cloud native team. Closes: keycloak#45805 Supersedes: keycloak#45806 Changes: - `Before you start` section newly mentions `config-api` sub-module and Quarkus word is capitalized - `Activating the Module from the root directory` section is dropped as it is not true: - Quarkus module is not enabled conditionally - Quarkus module compilation with JDK 11 fails as Quarkus baseline is JDK 17 and JDK version is enforced in Keycloak parent by `enforce-java-version` - `Building the Keycloak Quarkus distribution` section - capitalized Quarkus word - `Running from the command line` section contained link leading to unknown discussion https://github.com/keycloak/keycloak/discussions/categories/keycloak-x-quarkus-distribution, I replaced it with general GH discussion link. I could not verify the reload statement due to the issue described below, but judging by this discussion keycloak#14351 explanation about reload limitations is still valid - `Running tests` section used `Keycloaks` which I think was a typo, so I fixed it. And capitalized Quarkus word - `Running tests from your IDE` I changed statement about the `@DistributionTest` test being a blackbox, I think the build may happen in a JVM if it is container environment, otherwise it is just a different JVM process - `Running container-based tests` capitalized word `Docker`. I also dropped `The container based tests are using Testcontainers to spin up the container image and can be considered tech preview.` as 3 years seem long enough for container-based tests to mature - `Running database tests`, fixed few typos - `Updating Expectations` fixed command example and test class rendering - `CONTRIBUTING.md` is merged into `README.md` in following manner: - `Keycloak Quarkus Extension` section, renamed `Resteasy` to `Quarkus REST` and missing recorder method `configSessionFactory` with `configSessionFactory` - dropped `Running the server in development mode` section as it was dealing with the same topic as the `Running from the command line` `README.md` section. however I migrated few sentences - dropped `Running tests` as it deals with the same topic as `Running tests` section of the README and it contains everything the dropped section does, just in a different words - `Before contributing the changes` section content replaced the README `Contributing` section as it provides a better approach - `Documentation` fixed link to the server documentation guides What is not handled here: - `Running from the command line` section suggests command `../mvnw -f server/pom.xml compile quarkus:dev -Dkc.config.built=true -Dquarkus.args="start-dev"`, however running this command on my workstation leads to startup failure due to missing class `java.lang.ClassNotFoundException: org.osgi.framework.FrameworkUtil` - former `CONTRIBUTING.md` keeps stating that the bytecode is recorder during the runtime Signed-off-by: Michal Vavřík <michal.vavrik@aol.com>
|
@areevesat thanks again for raising this and suggesting the improvements. I'm closing this PR as it was superseded by #46033. Feel free to reach us out in case of any questions. |
Pull Request for #45805.
This pull request updates a few readme files to reflect that the shell commands are written for Linux, and suggests using a linux-like shell for users running in Windows.
This PR only contains documentation updates.