strimzi / strimzi-kafka-oauth

OAuth2 support for Apache Kafka® to work with many OAuth2 authorization servers
Apache License 2.0
140 stars 89 forks source link

Update README.md #239

Closed flo-kn closed 1 month ago

flo-kn commented 4 months ago

suggestion to fix path to sh script

closes #238

flo-kn commented 4 months ago

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:

https://github.com/strimzi/strimzi-kafka-oauth/blob/229daee85b096804d16e2904c8c0f1add599cc99/examples/docker/keycloak/compose.yml#L6

..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 commented 4 months ago

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.

mstruk commented 4 months ago

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 commented 3 months ago

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: https://github.com/strimzi/strimzi-kafka-oauth/blob/229daee85b096804d16e2904c8c0f1add599cc99/examples/README.md?plain=1#L20

flo-kn commented 3 months ago

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.

scholzj commented 1 month ago

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