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

Update README.md #239

Closed
wants to merge 1 commit into from
Closed

Update README.md #239

wants to merge 1 commit into from

Conversation

flo-kn
Copy link

@flo-kn flo-kn commented May 7, 2024

suggestion to fix path to sh script

closes #238

Signed-off-by: Florian Knip <104433967+flo-kn@users.noreply.github.com>
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

This likely relates to the container image that is used. So I do not think you can change it just like that without providing some more details.

@flo-kn
Copy link
Author

flo-kn commented May 8, 2024

It's dependent on the keycloak image, or image version. Wouldn't it make most sense -or at least be more intuitive - to have the path to the script similar to where it's located on the container instance that comes with what ever image is used here:

image: quay.io/keycloak/keycloak:23.0.5

..to serve consistency with rest of the example? That was my assumption here in this case.

docker ps
CONTAINER ID   IMAGE                              COMMAND                  CREATED        STATUS                         PORTS                                            NAMES
17512408edb1   quay.io/keycloak/keycloak:23.0.5   "/opt/keycloak/bin/k…"   14 hours ago   Up 14 hours                    0.0.0.0:8080->8080/tcp, 0.0.0.0:8443->8443/tcp   docker-keycloak-1
docker exec -ti docker-keycloak-1 /bin/sh
ls /opt/keycloak/bin/
client  federation-sssd-setup.sh  kcadm.bat  kcadm.sh  kc.bat  kcreg.bat  kcreg.sh  kc.sh

@scholzj
Copy link
Member

scholzj commented May 8, 2024

If that is the path in the images used in the examples then yes, that would make sense. But you have to properly explain it so that others understand it and not just change the path without any proper explanation here or in the issue.

@scholzj scholzj requested a review from mstruk May 8, 2024 08:16
@mstruk
Copy link
Contributor

mstruk commented May 8, 2024

The proposed fix looks good. When Keycloak container version was bumped last time (from pre-quarkus version of Keycloak) this line should have been adjusted as well.

A deeper question for @flo-kn - Do you find value in this README.md file? It seems to me to unnecessarily rely on Keycloak command line client in order to demonstrate how Keycloak generates different Payload section of JWT token using the predefine realm (clients and users already configured) and depending on the client / user used.

kcadm.sh tool here is used purely as a readily available OAuth client. The same can be achieved using curl / wget and would be more useful as these two are readily available in application container images that give access to shell.

@flo-kn
Copy link
Author

flo-kn commented May 10, 2024

To your question @mstruk Generally, I think it is good to have some lines in the README.md explaining how to verify functionality of the installation or in this case parts of the installation. If it's curl, wget, or kcadm.sh on the container instance depends on personal taste.

If you'd ask me for a suggestion to improve the readme a bit: In this case, I think setting another heading would be beneficial to give it more structure so that user knows in advance what next lines will be about:

## Verify Keycloak Functionality
Before we start with the demos the let's check first that keycloak is working correctly. 

To do that connect to 'keycloak' contai....

E.g. around this area would be a suitable place:

Assuming `keycloak` container is up and running you can do the following.

@flo-kn
Copy link
Author

flo-kn commented May 14, 2024

If that is the path in the images used in the examples then yes, that would make sense. But you have to properly explain it so that others understand it and not just change the path without any proper explanation here or in the issue.

@scholzj Just updated the issue description. Hope, it's ok like that. IMO adding more to the guide other then the correct directory does not bring any benefit. Lemme know what you think.

@flo-kn flo-kn requested a review from scholzj May 14, 2024 13:36
@scholzj
Copy link
Member

scholzj commented Jul 11, 2024

Discussed on the community call: @mstruk will take the ideas and use them as improvements to the README files. This PR should be closed.

@scholzj scholzj closed this Jul 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check path kcadm.sh in keycloak
3 participants