WIP ipa-tuura: add demo for ipa-tuura using IPA and Keycloak #24
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @f-trivino - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 10 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| usage() { | ||
| cat <<EOF | ||
| usage: $(basename "$0") [-p PROFILE] [-r] [URL]" |
There was a problem hiding this comment.
suggestion (typo): Remove stray trailing double quote in usage message
| usage: $(basename "$0") [-p PROFILE] [-r] [URL]" | |
| usage: $(basename "$0") [-p PROFILE] [-r] [URL] |
|
|
||
| WORKSHOP_PROFILE_DIR="${HOME}/.mozilla/firefox/${profile_name}" | ||
|
|
||
| if [ "$cmd" == "remove" ] |
There was a problem hiding this comment.
suggestion (bug_risk): Use single '=' for string comparison in POSIX sh
| if [ "$cmd" == "remove" ] | |
| if [ "$cmd" = "remove" ] |
|
|
||
| if [ "$cmd" == "remove" ] | ||
| then | ||
| sed -i "/^\# start - Added by ipalab-config: ${profile_name}$/,/^\# end - Added by ipalab-config: ${profile_name}/d" "${MOZILLA_PROFILES}" |
There was a problem hiding this comment.
suggestion (bug_risk): Mismatch between profile markers in sed removal and cat insertion
Replace 'ipalab-config' with 'ipa-demos' in the sed removal command so it matches the insertion marker.
| sed -i "/^\# start - Added by ipalab-config: ${profile_name}$/,/^\# end - Added by ipalab-config: ${profile_name}/d" "${MOZILLA_PROFILES}" | |
| sed -i "/^\# start - Added by ipa-demos: ${profile_name}$/,/^\# end - Added by ipa-demos: ${profile_name}/d" "${MOZILLA_PROFILES}" |
There was a problem hiding this comment.
ipalab-config is the name of the tool, don't replace it.
There was a problem hiding this comment.
You’re right—ipalab-config is the tool’s name, so the markers should stay as-is. My bad for the confusion; no change needed here.
| h) usage && exit 0 ;; | ||
| p) profile_name="${OPTARG}" ;; | ||
| r) cmd="remove" ;; | ||
| *) die -u "Invalid option: ${OPTARG}" ;; |
There was a problem hiding this comment.
suggestion: Unexpected '-u' flag passed to die()
Remove '-u' (die() doesn’t accept flags), or update die() to handle usage output.
| *) die -u "Invalid option: ${OPTARG}" ;; | |
| *) die "Invalid option: ${OPTARG}" ;; |
|
|
||
| fi | ||
|
|
||
| [ -z "$@" ] || detach podman unshare --rootless-netns firefox -P "$profile_name" --new-window "$@" |
There was a problem hiding this comment.
suggestion (bug_risk): Incorrect empty-arguments test with '-z "$@"'
Use '[
|
|
||
| Before starting Firefox, add the entries found in the generated `hosts` | ||
| file to your machine `/etc/hosts` so the host names can be resolved. The | ||
| file `hosts` has all the containers entries needed, add it with: |
There was a problem hiding this comment.
suggestion (typo): Consider rephrasing 'containers entries'.
Change “containers entries” to “container entries” for clarity.
| file `hosts` has all the containers entries needed, add it with: | |
| file `hosts` has all the container entries needed, add it with: |
There was a problem hiding this comment.
change to ".. has the entries for all containers .."
There was a problem hiding this comment.
Sounds good—“hosts” has the entries for all containers needed. I’ll update the README to:
Before starting Firefox, add the entries found in the generated `hosts` file to your machine’s `/etc/hosts` so the host names can be resolved. The `hosts` file has the entries for all containers needed, add it with:| -s 'config.keycloak_hostname=["keycloak.ipa.test"]' | ||
| ``` | ||
|
|
||
| CHECK IF WE CAN CREATE A JSON and rely on kcreg: |
There was a problem hiding this comment.
issue: Developer note found in documentation.
Remove or address the 'CHECK IF WE CAN...' developer note and update the documentation accordingly.
|
|
||
| ## Troubleshooting | ||
|
|
||
| ADD ipa-tuura troubleshooting |
There was a problem hiding this comment.
issue: Placeholder comment in Troubleshooting section.
Please replace the placeholder with the actual ipa-tuura troubleshooting steps or remove it.
This demo is based on the ipalab-config configuration of Keycloak, which is based on the official Keycloak container image, using a self-signed certificate for HTTPS connection. Keycloak runs in production mode. Helper scripts and playbooks for configuring Keycloak, IPA, and ipa-tuura are provided.
This demo is based on the ipalab-config configuration of Keycloak, which is based on the official Keycloak container image, using a self-signed certificate for HTTPS connection. Keycloak runs in production mode.
Helper scripts and playbooks for configuring Keycloak, IPA, and ipa-tuura are provided.
Summary by Sourcery
Add a self-contained demo for ipa-tuura integration with FreeIPA and Keycloak, including configuration files, helper scripts, and Ansible playbooks to deploy and test the environment
New Features:
Documentation:
Chores: