Open
Conversation
Closed
Contributor
Author
|
@streamer45 @cwarnermm - You can ignore this for the moment. I'm just trying to get it to publish a preview environment so i can look at how things lay out from the Claude Code changes. I'm not sure why I can't get this PR to build a preview |
cwarnermm
added
the
preview-environment
Contributor
|
Ah, I do, @sadohert. It's because it's a fork, not a branch. Only branches (via authorized users) trigger automated previews. For forks, we need to add the |
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA d0ee389 |
Contributor
|
@sadohert - Preview now available. |
sadohert
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA 0a722e5 |
cwarnermm
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA 39abca9 |
Contributor
Author
|
I'm transitioning these back to markdown. My bad. I thought RST was the standard we were moving to. I'll also resolve the merge conflict. |
sadohert
force-pushed
the
stu-calls-claude-05-07
branch
from
a1d3830 to
304864a
Compare
sadohert
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA 304864a |
sadohert
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA 52cc483 |
Contributor
Author
|
TODOs:
|
sadohert
added
preview-environment
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
source/administration-guide/configure/calls-offloader-setup.md (2)
575-586:⚠️ Potential issue | 🟠 MajorKeep the offloader config filename consistent.
Earlier in the guide, the install steps create
/opt/calls-offloader/config.tomland the systemd unit starts the service with that file, but these sections switch to/opt/calls-offloader/calls-offloader.toml. Following both sets of instructions leaves readers editing the wrong file during air-gap setup.Suggested doc fix
-Update `/opt/calls-offloader/calls-offloader.toml`: +Update `/opt/calls-offloader/config.toml`:-- Verify the `image_registry` configuration in calls-offloader.toml +- Verify the `image_registry` configuration in `/opt/calls-offloader/config.toml`-grep image_registry /opt/calls-offloader/calls-offloader.toml +grep image_registry /opt/calls-offloader/config.toml-sed -i "s|localhost:5000|$REGISTRY_HOST|g" /opt/calls-offloader/calls-offloader.toml +sed -i "s|localhost:5000|$REGISTRY_HOST|g" /opt/calls-offloader/config.tomlAlso applies to: 636-653, 694-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` around lines 575 - 586, The doc is inconsistent about the offloader config filename (uses /opt/calls-offloader/config.toml earlier but later shows /opt/calls-offloader/calls-offloader.toml); unify them by choosing one canonical name (preferably /opt/calls-offloader/config.toml) and update every occurrence including the configuration example under "Calls-Offloader Configuration" and the systemd unit invocation so they all reference the same file (ensure references in the image_registry example and the service start command match the chosen filename).
595-602:⚠️ Potential issue | 🟠 MajorThe verification flow shouldn’t require
latest.The manual setup path only tags and pushes explicit versioned images, so these
docker pull ...:latestchecks — and the later instruction to ensurelatestexists — will fail for anyone following the manual workflow exactly. Use the plugin-matched version tags here, or explicitly addlatestto the manual push steps.Suggested doc fix
-# Test pulling an image -docker pull localhost:5000/mattermost/calls-recorder:latest +# Test pulling an image +docker pull localhost:5000/mattermost/calls-recorder:<RECORDER_VERSION>-docker pull localhost:5000/mattermost/calls-recorder:latest -docker pull localhost:5000/mattermost/calls-transcriber:latest +docker pull localhost:5000/mattermost/calls-recorder:<RECORDER_VERSION> +docker pull localhost:5000/mattermost/calls-transcriber:<TRANSCRIBER_VERSION>-3. Ensure both versioned tags (e.g., `v0.8.5`) and `latest` tags are present in your local registry +3. Ensure the exact versioned tags expected by your Calls plugin are present in your local registryAlso applies to: 656-672
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` around lines 595 - 602, The verification steps currently use docker pull localhost:5000/mattermost/calls-recorder:latest which will fail for the manual push workflow that only tags versioned images; update the two verification blocks (around the curl http://localhost:5000/v2/_catalog and the docker pull lines, also the similar section at the later block referenced) to either pull the plugin-matched version tag (e.g., replace :latest with the explicit plugin version tag used in the push) or add a preceding instruction to tag and push the image as :latest during the manual push steps so the pull commands succeed.source/administration-guide/configure/calls-rtcd-setup.md (1)
358-363:⚠️ Potential issue | 🟠 MajorRemove the separate “Enable RTCD Service” step.
This flow tells admins to look for an extra toggle, but the Calls setup is driven by the RTCD Service URL field. Keeping the nonexistent “Enable RTCD Service” step here is likely to block configuration.
Suggested doc fix
1. Go to **System Console > Plugins > Calls** - -2. Enable the **Enable RTCD Service** option - -3. Set the **RTCD Service URL** to your RTCD service address (either a single server or DNS load-balanced hostname). Ensure you provide any generated credentials formulated in the URI (e.g., `http://clientID:authKey@rtcd.local`). For detailed capability/credential configuration, reference the [RTCD Service URL](https://docs.mattermost.com/configure/plugins-configuration-settings.html#rtcd-service-url) documentation. +2. Set the **RTCD Service URL** to your RTCD service address (either a single server or DNS load-balanced hostname). Ensure you provide any generated credentials formulated in the URI (e.g., `http://clientID:authKey@rtcd.local`). For detailed capability/credential configuration, reference the [RTCD Service URL](https://docs.mattermost.com/configure/plugins-configuration-settings.html#rtcd-service-url) documentation. -4. Save the configuration +3. Save the configuration -5. Test by creating a new call in any Mattermost channel +4. Test by creating a new call in any Mattermost channel -6. Verify that the call is being routed through RTCD by checking the RTCD logs and metrics +5. Verify that the call is being routed through RTCD by checking the RTCD logs and metrics🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-rtcd-setup.md` around lines 358 - 363, Remove the separate "Enable RTCD Service" step from the numbered flow under "System Console > Plugins > Calls" and update the instructions to indicate that configuration is driven solely by the "RTCD Service URL" field (provide credentials in the URI as shown, e.g., http://clientID:authKey@rtcd.local); ensure the step list flows directly from opening Plugins > Calls to setting the RTCD Service URL and referencing the RTCD Service URL documentation, and delete any references to an "Enable RTCD Service" toggle or step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/administration-guide/configure/calls-offloader-setup.md`:
- Around line 531-536: The current doc tells admins to overwrite the Docker
daemon config by echoing a single-field JSON into /etc/docker/daemon.json (the
command "echo '{\"insecure-registries\": [\"localhost:5000\"]}' | sudo tee
/etc/docker/daemon.json"), which can silently discard existing settings; change
the instructions to (1) instruct users to back up the existing file first, (2)
show a safe merge approach (e.g., parse existing /etc/docker/daemon.json and
merge or append the insecure-registries entry using a JSON-aware tool like jq,
or instruct manual editing to add/merge the "insecure-registries" array), and
(3) replace the overwriting command with the backup + merge/manual edit
guidance; apply the same replacement to the other identical echo-to-daemon.json
snippets in the document.
- Around line 407-427: Text incorrectly refers to "calls-offloader" instead of
the actual image name "calls-recorder"; update the guidance so it instructs
admins to match the calls-recorder and calls-transcriber images to the versions
in plugin.json by reading the calls_recorder_version and
calls_transcriber_version keys (instead of referencing calls-offloader), and
ensure any earlier mentions or examples in this section use the exact image name
"calls-recorder" to avoid pointing users to the wrong artifact.
In `@source/administration-guide/configure/calls-rtcd-setup.md`:
- Around line 296-317: After the UDP connectivity test (after the nc
server/client examples that require stopping the RTCD service), add a step to
restart the service: run sudo systemctl start rtcd so the rtcd service is
brought back up for subsequent validation and integration; ensure this follows
the nc test instructions and references the same rtcd service that was stopped
with sudo systemctl stop rtcd.
---
Duplicate comments:
In `@source/administration-guide/configure/calls-offloader-setup.md`:
- Around line 575-586: The doc is inconsistent about the offloader config
filename (uses /opt/calls-offloader/config.toml earlier but later shows
/opt/calls-offloader/calls-offloader.toml); unify them by choosing one canonical
name (preferably /opt/calls-offloader/config.toml) and update every occurrence
including the configuration example under "Calls-Offloader Configuration" and
the systemd unit invocation so they all reference the same file (ensure
references in the image_registry example and the service start command match the
chosen filename).
- Around line 595-602: The verification steps currently use docker pull
localhost:5000/mattermost/calls-recorder:latest which will fail for the manual
push workflow that only tags versioned images; update the two verification
blocks (around the curl http://localhost:5000/v2/_catalog and the docker pull
lines, also the similar section at the later block referenced) to either pull
the plugin-matched version tag (e.g., replace :latest with the explicit plugin
version tag used in the push) or add a preceding instruction to tag and push the
image as :latest during the manual push steps so the pull commands succeed.
In `@source/administration-guide/configure/calls-rtcd-setup.md`:
- Around line 358-363: Remove the separate "Enable RTCD Service" step from the
numbered flow under "System Console > Plugins > Calls" and update the
instructions to indicate that configuration is driven solely by the "RTCD
Service URL" field (provide credentials in the URI as shown, e.g.,
http://clientID:authKey@rtcd.local); ensure the step list flows directly from
opening Plugins > Calls to setting the RTCD Service URL and referencing the RTCD
Service URL documentation, and delete any references to an "Enable RTCD Service"
toggle or step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 644e9285-cb29-40d4-a4b8-199d3339b606
📒 Files selected for processing (2)
source/administration-guide/configure/calls-offloader-setup.mdsource/administration-guide/configure/calls-rtcd-setup.md
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
source/administration-guide/configure/calls-offloader-setup.md (3)
407-407:⚠️ Potential issue | 🟡 MinorFix incorrect image name in version-matching guidance.
The text incorrectly refers to
calls-offloaderwhen it should saycalls-recorder. The Docker images used aremattermost/calls-recorderandmattermost/calls-transcriber, notcalls-offloader.Suggested fix
-**Important**: The exact versions of `calls-offloader` and `calls-transcriber` images must match what your installed Calls plugin expects. These versions are defined in the Calls plugin source code. +**Important**: The exact versions of `calls-recorder` and `calls-transcriber` images must match what your installed Calls plugin expects. These versions are defined in the Calls plugin source code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` at line 407, Update the sentence that currently references `calls-offloader` to correctly refer to `calls-recorder`; change the guidance so it states that the Docker images must match the Calls plugin and list the correct image names `mattermost/calls-recorder` and `mattermost/calls-transcriber` instead of `calls-offloader`, ensuring the text around version-matching (the line mentioning versions defined in the Calls plugin source) reflects these exact image names.
578-594:⚠️ Potential issue | 🟠 MajorFix config filename inconsistency.
The air-gap manual configuration section references
/opt/calls-offloader/calls-offloader.toml, but the standard installation instructions at lines 69 and 114 create and use/opt/calls-offloader/config.toml. Following both sections as written leaves the active config unchanged during air-gapped setup.Suggested fix
#### 2. Calls-Offloader Configuration -Update `/opt/calls-offloader/calls-offloader.toml`: +Update `/opt/calls-offloader/config.toml`: ```toml [jobs]Also update the references at lines 648, 656, and 691 to use
config.tomlconsistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` around lines 578 - 594, The document references two different config filenames causing confusion—replace all occurrences of "calls-offloader.toml" with the consistent filename "config.toml" in the air-gap configuration section so the example that updates the [jobs] image_registry (the block currently showing image_registry = "localhost:5000/mattermost") edits the same file used by the standard installation; update the three other references mentioned (the later mentions at lines near the image_registry examples) to "config.toml" as well and confirm the restart instruction still points to restarting the calls-offloader service.
601-605:⚠️ Potential issue | 🟡 MinorUse versioned tags in verification instead of
:latest.The manual setup workflow at lines 472-483 only tags and pushes versioned images (e.g.,
v0.8.5), but this verification step pulls:latest. Users following the manual path will see a false failure. Use the explicit versioned tag that matches the manual workflow.Suggested fix
# Test pulling an image -docker pull localhost:5000/mattermost/calls-recorder:latest +docker pull localhost:5000/mattermost/calls-recorder:<VERSION>Where
<VERSION>matches the version used in the manual setup (e.g.,v0.8.5).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` around lines 601 - 605, Replace the verification pull that uses the floating tag docker pull localhost:5000/mattermost/calls-recorder:latest with a pull that uses the explicit version tag used earlier (e.g., docker pull localhost:5000/mattermost/calls-recorder:<VERSION>), and ensure the <VERSION> you use matches the tag you created and pushed in the manual setup steps; similarly update any examples or commands that reference :latest for this image to use the same versioned tag.scripts/air-gap-docker-registry-setup.sh (5)
328-347:⚠️ Potential issue | 🟠 MajorExit or gate configuration steps when internet check fails.
When
check_internet()fails, the script prints a message but continues to runconfigure_docker_daemon(),configure_calls_offloader(), andverify_setup(). This mutates the host's Docker daemon configuration beforeverify_setup()inevitably fails (since no registry was set up and no images were downloaded).Suggested fix
# Check if we have internet access for image pulling if ! check_internet; then echo "Skipping image download phase - run this script with internet access first" | tee -a $LOG_FILE + echo "Exiting. Re-run this script when internet access is available." | tee -a $LOG_FILE + exit 1 - else - # Setup local registry - setup_registry - - # Download and push images - setup_mattermost_images fi - # Configure Docker daemon - configure_docker_daemon + # Setup local registry + setup_registry - # Configure calls-offloader - configure_calls_offloader + # Download and push images + setup_mattermost_images - # Verify setup - verify_setup + # Configure Docker daemon + configure_docker_daemon + ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 328 - 347, If check_internet() fails, the script must not continue to configuration/verification steps; update the control flow in the main block so that when ! check_internet returns true you either exit the script (e.g., exit 1) or skip subsequent actions by gating calls to configure_docker_daemon, configure_calls_offloader, and verify_setup behind the successful internet/registry branch; locate the block around check_internet, setup_registry, setup_mattermost_images and ensure that only the else branch runs setup_registry, setup_mattermost_images, configure_docker_daemon, configure_calls_offloader, and verify_setup (or call exit immediately after the echo) so the host Docker daemon is not mutated when internet check fails.
256-264:⚠️ Potential issue | 🔴 CriticalFix verification to pull an image that was actually pushed.
setup_mattermost_images()only pushesmattermost/calls-recorderandmattermost/calls-transcriber(from the IMAGES array at lines 142-145), but verification attempts to pullmattermost/calls-offloader:latest. This causes the verification to fail even after successful registry setup.Suggested fix
# Test pulling from local registry - test_image="$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-offloader:latest" - echo "Testing pull from local registry: $test_image" | tee -a $LOG_FILE - if docker pull $test_image >> $LOG_FILE 2>&1; then - echo "✓ Successfully pulled test image from local registry" | tee -a $LOG_FILE - else - echo "✗ Failed to pull test image from local registry" | tee -a $LOG_FILE - return 1 - fi + for test_image in \ + "$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-recorder:$CALLS_RECORDER_VERSION" \ + "$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-transcriber:$CALLS_TRANSCRIBER_VERSION" + do + echo "Testing pull from local registry: $test_image" | tee -a $LOG_FILE + if ! docker pull "$test_image" >> $LOG_FILE 2>&1; then + echo "✗ Failed to pull test image: $test_image" | tee -a $LOG_FILE + return 1 + fi + done + echo "✓ Successfully pulled test images from local registry" | tee -a $LOG_FILE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 256 - 264, The test pull uses a hardcoded test_image "mattermost/calls-offloader:latest" but setup_mattermost_images() only pushes images from the IMAGES array (e.g., mattermost/calls-recorder and mattermost/calls-transcriber), causing false failures; update the verification to pull an image that was actually pushed — either pick a specific pushed image like "mattermost/calls-recorder:latest" or derive the test image from the IMAGES array (e.g., use IMAGES[0] or the last pushed image) when building test_image and use that value in the docker pull check so verification matches what setup_mattermost_images() pushes.
193-204:⚠️ Potential issue | 🟠 MajorUse the correct config filename that matches installation docs.
The script looks for and edits
/opt/calls-offloader/calls-offloader.toml, but the installation documentation creates/opt/calls-offloader/config.tomland the systemd service is configured with--config /opt/calls-offloader/config.toml. The registry changes will never reach the active configuration.Suggested fix
- if [ -f "/opt/calls-offloader/calls-offloader.toml" ]; then - sudo cp /opt/calls-offloader/calls-offloader.toml /opt/calls-offloader/calls-offloader.toml.backup + if [ -f "/opt/calls-offloader/config.toml" ]; then + sudo cp /opt/calls-offloader/config.toml /opt/calls-offloader/config.toml.backup # Update image_registry setting - sudo sed -i "s/image_registry = \"mattermost\"/image_registry = \"$REGISTRY_HOST:$REGISTRY_PORT\/mattermost\"/" /opt/calls-offloader/calls-offloader.toml + sudo sed -i "s/image_registry = \"mattermost\"/image_registry = \"$REGISTRY_HOST:$REGISTRY_PORT\/mattermost\"/" /opt/calls-offloader/config.toml echo "Updated calls-offloader configuration to use local registry" | tee -a $LOG_FILE - echo "Backup created at /opt/calls-offloader/calls-offloader.toml.backup" | tee -a $LOG_FILE + echo "Backup created at /opt/calls-offloader/config.toml.backup" | tee -a $LOG_FILE else - echo "Warning: calls-offloader.toml not found. You'll need to manually configure:" | tee -a $LOG_FILE + echo "Warning: config.toml not found. You'll need to manually configure:" | tee -a $LOG_FILEAlso apply the same fix at lines 293-304 in
create_airgap_deployment_script().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 193 - 204, The script is editing /opt/calls-offloader/calls-offloader.toml but the active config is /opt/calls-offloader/config.toml; update the existence check, backup, sed replacement, and echo messages to operate on config.toml (e.g., change references from calls-offloader.toml to config.toml and back up to config.toml.backup) so the image_registry replacement actually affects the running service; also apply the identical change inside the create_airgap_deployment_script() block where the same logic is repeated.
220-228:⚠️ Potential issue | 🟠 MajorMerge
insecure-registriesinto existing daemon.json instead of replacing it.The script backs up the existing
daemon.jsonbut then overwrites it with a minimal configuration. This can silently discard existing settings like mirrors, logging, storage-driver, or runtime configurations. Consider usingjqto merge theinsecure-registriesentry when the file exists.Suggested fix
# Create new daemon.json with insecure registry configuration - # Note: If an existing daemon.json exists, these settings should be merged manually instead. - cat > /tmp/daemon.json << EOF + if [ -f "/etc/docker/daemon.json" ] && command -v jq &> /dev/null; then + # Merge insecure-registries into existing config + jq --arg registry "$REGISTRY_HOST:$REGISTRY_PORT" \ + '.["insecure-registries"] = (.["insecure-registries"] // []) + [$registry] | .["insecure-registries"] |= unique' \ + /etc/docker/daemon.json.backup > /tmp/daemon.json + else + # Create new daemon.json (warn user if overwriting) + if [ -f "/etc/docker/daemon.json" ]; then + echo "Warning: jq not available. Overwriting daemon.json. Review backup at /etc/docker/daemon.json.backup" | tee -a $LOG_FILE + fi + cat > /tmp/daemon.json << EOF { "insecure-registries": ["$REGISTRY_HOST:$REGISTRY_PORT"] } EOF + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 220 - 228, Currently the script unconditionally replaces /etc/docker/daemon.json with a minimal JSON, discarding existing settings; change it to detect if /etc/docker/daemon.json exists and, if so, use jq to merge/append the insecure-registries entry (constructed from "$REGISTRY_HOST:$REGISTRY_PORT") into the existing JSON (deduplicating entries) writing the result to a temp file (e.g., /tmp/daemon.json) and then atomically mv it into place; if the file does not exist, create the minimal JSON as before; keep the existing backup logic and ensure failures from jq are handled before moving the temp file.
293-305:⚠️ Potential issue | 🟠 MajorApply same config filename fix in generated deployment script.
The generated deployment script also uses
calls-offloader.tomlinstead ofconfig.toml, which needs to be consistent with the main script fix.Suggested fix
# Update calls-offloader configuration -if [ -f "/opt/calls-offloader/calls-offloader.toml" ]; then - sudo sed -i "s/image_registry = \"mattermost\"/image_registry = \"$REGISTRY_HOST:$REGISTRY_PORT\/mattermost\"/" /opt/calls-offloader/calls-offloader.toml +if [ -f "/opt/calls-offloader/config.toml" ]; then + sudo sed -i "s/image_registry = \"mattermost\"/image_registry = \"$REGISTRY_HOST:$REGISTRY_PORT\/mattermost\"/" /opt/calls-offloader/config.toml # Restart calls-offloader service sudo systemctl restart calls-offloader echo "Calls-offloader configured for air-gap deployment" else - echo "Warning: /opt/calls-offloader/calls-offloader.toml not found" + echo "Warning: /opt/calls-offloader/config.toml not found"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 293 - 305, The generated deployment script is updating /opt/calls-offloader/calls-offloader.toml but should use /opt/calls-offloader/config.toml to match the main script; update the sed invocation that replaces image_registry (the line using REGISTRY_HOST and REGISTRY_PORT) to target config.toml instead of calls-offloader.toml, change the existence check and the warning/echo messages to reference /opt/calls-offloader/config.toml, and keep the sudo systemctl restart calls-offloader call as-is so the service is restarted after the change.source/administration-guide/configure/calls-rtcd-setup.md (1)
364-370:⚠️ Potential issue | 🟠 MajorRemove or clarify the "Enable RTCD Service" step.
The Calls plugin settings expose the RTCD Service URL field, not a separate Enable RTCD Service toggle. Telling admins to look for an extra toggle that doesn't exist will block their setup. Consider consolidating steps 2 and 3 into a single step that instructs setting the RTCD Service URL.
Suggested fix
1. Go to **System Console > Plugins > Calls** -2. Enable the **Enable RTCD Service** option - -3. Set the **RTCD Service URL** to your RTCD service address (either a single server or DNS load-balanced hostname). Ensure you provide any generated credentials formulated in the URI (e.g., `http://clientID:authKey@rtcd.local`). For detailed capability/credential configuration, reference the [RTCD Service URL](https://docs.mattermost.com/configure/plugins-configuration-settings.html#rtcd-service-url) documentation. +2. Set the **RTCD Service URL** to your RTCD service address (either a single server or DNS load-balanced hostname). Ensure you provide any generated credentials formulated in the URI (e.g., `http://clientID:authKey@rtcd.local`). For detailed capability/credential configuration, reference the [RTCD Service URL](https://docs.mattermost.com/configure/plugins-configuration-settings.html#rtcd-service-url) documentation. -4. Save the configuration +3. Save the configuration -5. Test by creating a new call in any Mattermost channel +4. Test by creating a new call in any Mattermost channel -6. Verify that the call is being routed through RTCD by checking the RTCD logs and metrics +5. Verify that the call is being routed through RTCD by checking the RTCD logs and metrics🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-rtcd-setup.md` around lines 364 - 370, The guide incorrectly references an "Enable RTCD Service" toggle; remove that step and consolidate steps 2 and 3 into a single instruction that tells admins to set the RTCD Service URL (the "RTCD Service URL" field) to their RTCD service address, including any credentials in the URI (e.g., http://clientID:authKey@rtcd.local), then save the configuration; update the text that mentions "Enable RTCD Service" so only the "RTCD Service URL" field is referenced.
🧹 Nitpick comments (1)
scripts/air-gap-docker-registry-setup.sh (1)
232-232: Useteefor redirects withsudocommands.The
sudocommand doesn't affect the redirect, so if the log file required elevated permissions, this would fail. While/tmp/is typically writable, usingtee -ais more robust.Suggested fix
- sudo systemctl restart docker >> $LOG_FILE 2>&1 + sudo systemctl restart docker 2>&1 | tee -a $LOG_FILE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` at line 232, Replace the bare redirect on the sudo command so the elevated command's output is captured even if the log file needs root permissions; change the line using "sudo systemctl restart docker >> $LOG_FILE 2>&1" to pipe stderr/stdout into tee with sudo, e.g. run "sudo systemctl restart docker 2>&1 | sudo tee -a \"$LOG_FILE\" >/dev/null" so the command output is appended to the log with proper privileges.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/administration-guide/configure/calls-rtcd-setup.md`:
- Around line 172-178: Update the ordered list numbering for the step containing
the systemctl commands so it reads "6." instead of "5." — locate the numbered
list item that includes the three commands (sudo systemctl daemon-reload / sudo
systemctl enable rtcd / sudo systemctl start rtcd) and change its prefix from 5.
to 6. to correct the duplicate numbering in configure/calls-rtcd-setup.md.
- Around line 89-90: The sentence describing HTTP/WebSocket connectivity to the
rtcd service uses the incorrect word "expose"; update the text in the sentence
that mentions "Calls plugin" and "<code>rtcd</code>" to use "exposed" instead of
"expose" so it reads "...Can be exposed internally as the service only needs to
be reachable by the instance(s) running the Mattermost server." Ensure only the
typo is corrected and punctuation/HTML tags remain unchanged.
---
Duplicate comments:
In `@scripts/air-gap-docker-registry-setup.sh`:
- Around line 328-347: If check_internet() fails, the script must not continue
to configuration/verification steps; update the control flow in the main block
so that when ! check_internet returns true you either exit the script (e.g.,
exit 1) or skip subsequent actions by gating calls to configure_docker_daemon,
configure_calls_offloader, and verify_setup behind the successful
internet/registry branch; locate the block around check_internet,
setup_registry, setup_mattermost_images and ensure that only the else branch
runs setup_registry, setup_mattermost_images, configure_docker_daemon,
configure_calls_offloader, and verify_setup (or call exit immediately after the
echo) so the host Docker daemon is not mutated when internet check fails.
- Around line 256-264: The test pull uses a hardcoded test_image
"mattermost/calls-offloader:latest" but setup_mattermost_images() only pushes
images from the IMAGES array (e.g., mattermost/calls-recorder and
mattermost/calls-transcriber), causing false failures; update the verification
to pull an image that was actually pushed — either pick a specific pushed image
like "mattermost/calls-recorder:latest" or derive the test image from the IMAGES
array (e.g., use IMAGES[0] or the last pushed image) when building test_image
and use that value in the docker pull check so verification matches what
setup_mattermost_images() pushes.
- Around line 193-204: The script is editing
/opt/calls-offloader/calls-offloader.toml but the active config is
/opt/calls-offloader/config.toml; update the existence check, backup, sed
replacement, and echo messages to operate on config.toml (e.g., change
references from calls-offloader.toml to config.toml and back up to
config.toml.backup) so the image_registry replacement actually affects the
running service; also apply the identical change inside the
create_airgap_deployment_script() block where the same logic is repeated.
- Around line 220-228: Currently the script unconditionally replaces
/etc/docker/daemon.json with a minimal JSON, discarding existing settings;
change it to detect if /etc/docker/daemon.json exists and, if so, use jq to
merge/append the insecure-registries entry (constructed from
"$REGISTRY_HOST:$REGISTRY_PORT") into the existing JSON (deduplicating entries)
writing the result to a temp file (e.g., /tmp/daemon.json) and then atomically
mv it into place; if the file does not exist, create the minimal JSON as before;
keep the existing backup logic and ensure failures from jq are handled before
moving the temp file.
- Around line 293-305: The generated deployment script is updating
/opt/calls-offloader/calls-offloader.toml but should use
/opt/calls-offloader/config.toml to match the main script; update the sed
invocation that replaces image_registry (the line using REGISTRY_HOST and
REGISTRY_PORT) to target config.toml instead of calls-offloader.toml, change the
existence check and the warning/echo messages to reference
/opt/calls-offloader/config.toml, and keep the sudo systemctl restart
calls-offloader call as-is so the service is restarted after the change.
In `@source/administration-guide/configure/calls-offloader-setup.md`:
- Line 407: Update the sentence that currently references `calls-offloader` to
correctly refer to `calls-recorder`; change the guidance so it states that the
Docker images must match the Calls plugin and list the correct image names
`mattermost/calls-recorder` and `mattermost/calls-transcriber` instead of
`calls-offloader`, ensuring the text around version-matching (the line
mentioning versions defined in the Calls plugin source) reflects these exact
image names.
- Around line 578-594: The document references two different config filenames
causing confusion—replace all occurrences of "calls-offloader.toml" with the
consistent filename "config.toml" in the air-gap configuration section so the
example that updates the [jobs] image_registry (the block currently showing
image_registry = "localhost:5000/mattermost") edits the same file used by the
standard installation; update the three other references mentioned (the later
mentions at lines near the image_registry examples) to "config.toml" as well and
confirm the restart instruction still points to restarting the calls-offloader
service.
- Around line 601-605: Replace the verification pull that uses the floating tag
docker pull localhost:5000/mattermost/calls-recorder:latest with a pull that
uses the explicit version tag used earlier (e.g., docker pull
localhost:5000/mattermost/calls-recorder:<VERSION>), and ensure the <VERSION>
you use matches the tag you created and pushed in the manual setup steps;
similarly update any examples or commands that reference :latest for this image
to use the same versioned tag.
In `@source/administration-guide/configure/calls-rtcd-setup.md`:
- Around line 364-370: The guide incorrectly references an "Enable RTCD Service"
toggle; remove that step and consolidate steps 2 and 3 into a single instruction
that tells admins to set the RTCD Service URL (the "RTCD Service URL" field) to
their RTCD service address, including any credentials in the URI (e.g.,
http://clientID:authKey@rtcd.local), then save the configuration; update the
text that mentions "Enable RTCD Service" so only the "RTCD Service URL" field is
referenced.
---
Nitpick comments:
In `@scripts/air-gap-docker-registry-setup.sh`:
- Line 232: Replace the bare redirect on the sudo command so the elevated
command's output is captured even if the log file needs root permissions; change
the line using "sudo systemctl restart docker >> $LOG_FILE 2>&1" to pipe
stderr/stdout into tee with sudo, e.g. run "sudo systemctl restart docker 2>&1 |
sudo tee -a \"$LOG_FILE\" >/dev/null" so the command output is appended to the
log with proper privileges.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 453248df-1d35-4780-9ad7-72768bae1efd
📒 Files selected for processing (5)
scripts/air-gap-docker-registry-setup.shsource/administration-guide/configure/calls-offloader-setup.mdsource/administration-guide/configure/calls-rtcd-setup.mdsource/administration-guide/configure/calls-troubleshooting.mdsource/deployment-guide/server/containers/install-docker.rst
🚧 Files skipped from review as they are similar to previous changes (1)
- source/administration-guide/configure/calls-troubleshooting.md
sadohert
force-pushed
the
stu-calls-claude-05-07
branch
from
7e172f1 to
6d49b4d
Compare
…d bash script control flow)
There was a problem hiding this comment.
🧹 Nitpick comments (9)
source/administration-guide/configure/calls-offloader-setup.md (3)
796-851: Hardcoded versions in embedded script will become stale.The
update-air-gap-images.shscript template has hardcoded version strings (v0.8.5,v0.6.3) that won't match the user's actual plugin requirements. Consider either parameterizing the script or adding a prominent note that users must update these values.📝 Suggested improvement
cat > update-air-gap-images.sh << 'EOF' #!/bin/bash # Air-Gap Image Update Script set -e - NEW_RECORDER_VERSION="v0.8.5" - NEW_TRANSCRIBER_VERSION="v0.6.3" + # IMPORTANT: Update these versions to match your Calls plugin requirements + # Find correct versions in your plugin.json file + NEW_RECORDER_VERSION="<RECORDER_VERSION>" + NEW_TRANSCRIBER_VERSION="<TRANSCRIBER_VERSION>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` around lines 796 - 851, The embedded script update-air-gap-images.sh currently hardcodes NEW_RECORDER_VERSION ("v0.8.5") and NEW_TRANSCRIBER_VERSION ("v0.6.3"); change it to accept these as parameters or environment variables and update the documentation to call out that users must set NEW_RECORDER_VERSION and NEW_TRANSCRIBER_VERSION (or export REGISTRY_HOST/REGISTRY_PORT) before running the script; specifically, modify the script template to read versions from $1/$2 or from ${NEW_RECORDER_VERSION:-} and ${NEW_TRANSCRIBER_VERSION:-} and add a prominent note in the docs explaining how to supply or override these values prior to running update-air-gap-images.sh.
673-677: Minor grammar improvement needed.Line 676 is missing "that" before "both".
📝 Suggested fix
- 3. Ensure both versioned tags (e.g., `v0.8.5`) and `latest` tags are present in your local registry + 3. Ensure that both versioned tags (e.g., `v0.8.5`) and `latest` tags are present in your local registry🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` around lines 673 - 677, Change the sentence that reads "Ensure both versioned tags (e.g., `v0.8.5`) and `latest` tags are present in your local registry" to include the missing word "that" so it reads "Ensure that both versioned tags (e.g., `v0.8.5`) and `latest` tags are present in your local registry"; update this in the Calls offloader setup section of configure/calls-offloader-setup.md where the bullet "Ensure both versioned tags..." appears.
734-734: Minor grammar: missing article.📝 Suggested fix
-1. **On internet-connected machine**, run the air-gap setup script with new versions: +1. **On an internet-connected machine**, run the air-gap setup script with new versions:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-offloader-setup.md` at line 734, The phrase "On internet-connected machine" is missing the article; update the sentence to "On an internet-connected machine, run the air-gap setup script with new versions:" so the text string "On internet-connected machine" becomes "On an internet-connected machine" near the "air-gap setup script" instruction.source/administration-guide/configure/calls-rtcd-setup.md (3)
254-261: TOML indentation is inconsistent.The
ice_serversarray has inconsistent indentation—there's extra leading whitespace before it, which may confuse readers copying the configuration.📝 Suggested fix
[rtc] # TURN server configuration - ice_servers = [ - { urls = ["turn:turn.example.com:3478"], username = "turnuser", credential = "turnpassword" } - ] +ice_servers = [ + { urls = ["turn:turn.example.com:3478"], username = "turnuser", credential = "turnpassword" } +]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-rtcd-setup.md` around lines 254 - 261, The TOML snippet under the [rtc] section has inconsistent indentation for the ice_servers array; remove the extra leading whitespace so the ice_servers key and its value are aligned with the section header (i.e., make the ice_servers line and its array entries have consistent indentation under [rtc]), ensuring the identifier ice_servers and the [rtc] block remain unchanged and the array items (urls/username/credential) are indented consistently.
108-108: Minor grammar: consider adding a comma.Adding a comma after "environments" improves readability.
📝 Suggested fix
-This is the recommended deployment method for non-Kubernetes production environments as it provides the best performance and operational control. +This is the recommended deployment method for non-Kubernetes production environments, as it provides the best performance and operational control.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-rtcd-setup.md` at line 108, The sentence "This is the recommended deployment method for non-Kubernetes production environments as it provides the best performance and operational control." needs a comma after "environments" to improve readability; update the sentence in configure/calls-rtcd-setup.md (the line containing that sentence) to read "...for non-Kubernetes production environments, as it provides the best performance and operational control."
19-45: Consider moving inline CSS to a shared stylesheet.Embedding raw CSS within the Markdown file works but creates maintenance overhead if multiple docs need consistent table styling. If the documentation framework supports shared stylesheets or CSS includes, consider centralizing this styling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-rtcd-setup.md` around lines 19 - 45, The inline CSS for table styling (selectors like table.network-requirements, table.network-requirements th/td, and body:not([data-custom-theme="light"]) table.network-requirements ...) should be removed from the Markdown and moved into the project's central/shared stylesheet or a documentation CSS include; create or add these rules to the shared CSS module used by docs, update any import/includes so pages using the network-requirements table get the styles, and delete the embedded <style> block from configure/calls-rtcd-setup.md to avoid duplicate definitions and ease maintenance.scripts/air-gap-docker-registry-setup.sh (3)
124-134: Registry readiness check could be more robust.The fixed 10-second sleep may not be sufficient on slower systems, and the single curl check doesn't retry on failure. Consider a retry loop with a timeout.
📝 Suggested improvement
# Wait for registry to be ready echo "Waiting for registry to be ready..." | tee -a $LOG_FILE - sleep 10 - - # Test registry - if curl -s http://$REGISTRY_HOST:$REGISTRY_PORT/v2/ > /dev/null; then + local max_attempts=30 + local attempt=1 + while [ $attempt -le $max_attempts ]; do + if curl -s http://$REGISTRY_HOST:$REGISTRY_PORT/v2/ > /dev/null; then + echo "Local Docker registry is running at $REGISTRY_HOST:$REGISTRY_PORT" | tee -a $LOG_FILE + return 0 + fi + echo " Waiting for registry (attempt $attempt/$max_attempts)..." | tee -a $LOG_FILE + sleep 2 + attempt=$((attempt + 1)) + done + + echo "ERROR: Failed to start local Docker registry" | tee -a $LOG_FILE + return 1 - echo "Local Docker registry is running at $REGISTRY_HOST:$REGISTRY_PORT" | tee -a $LOG_FILE - else - echo "ERROR: Failed to start local Docker registry" | tee -a $LOG_FILE - return 1 - fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 124 - 134, Replace the fixed sleep + single curl check with a retry loop that polls the registry endpoint http://$REGISTRY_HOST:$REGISTRY_PORT/v2/ until it becomes healthy or a configurable timeout is reached: use LOG_FILE for logging, attempt curl repeatedly (e.g., every 2s with a maximum timeout or exponential backoff), print progress messages to tee -a $LOG_FILE, and if the timeout is exceeded return 1; update the block that currently references REGISTRY_HOST, REGISTRY_PORT, and LOG_FILE to implement this loop and timeout behavior instead of the single sleep and one-off curl.
257-264: Verification pull uses:latesttag which depends on tagging logic.The verification step pulls
calls-recorder:latest, which works because lines 165-168 create a:latesttag. However, if someone modifies the script to skip the latest tagging, verification would fail. Consider verifying with the explicit version tag that was definitely pushed.📝 Suggested improvement
# Test pulling from local registry - test_image="$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-recorder:latest" + test_image="$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-recorder:$CALLS_RECORDER_VERSION" echo "Testing pull from local registry: $test_image" | tee -a $LOG_FILE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 257 - 264, The verification pull should use the explicit tag that was pushed rather than hardcoding :latest; update the test_image construction (currently using test_image="$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-recorder:latest") to reference the version/tag variable used when tagging/pushing (e.g., IMAGE_TAG or PUSH_TAG), falling back to :latest only if that variable is empty, and keep logging to $LOG_FILE; this ensures the docker pull verifies the exact image that was pushed.
153-158: Quote shell variables to prevent word splitting.Variables used in docker commands should be quoted to handle edge cases safely.
📝 Suggested fix
# Pull from Docker Hub echo " Pulling $image:$version from Docker Hub..." | tee -a $LOG_FILE - docker pull $image:$version >> $LOG_FILE 2>&1 + docker pull "$image:$version" >> $LOG_FILE 2>&1 # Tag for local registry local_tag="$REGISTRY_HOST:$REGISTRY_PORT/$image:$version" echo " Tagging as $local_tag..." | tee -a $LOG_FILE - docker tag $image:$version $local_tag >> $LOG_FILE 2>&1 + docker tag "$image:$version" "$local_tag" >> $LOG_FILE 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/air-gap-docker-registry-setup.sh` around lines 153 - 158, Quote shell variables to prevent word splitting and globbing: update the docker pull and docker tag commands to use "$image:$version" and "$local_tag" and wrap redirections using "$LOG_FILE"; build the local_tag using quoted/brace-expanded variables (e.g., local_tag="${REGISTRY_HOST}:${REGISTRY_PORT}/${image}:${version}") so that REGISTRY_HOST, REGISTRY_PORT, image, and version are safe when they contain spaces or special chars; ensure all echo/tee and docker invocations reference "$local_tag" and "$LOG_FILE" consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/air-gap-docker-registry-setup.sh`:
- Around line 124-134: Replace the fixed sleep + single curl check with a retry
loop that polls the registry endpoint http://$REGISTRY_HOST:$REGISTRY_PORT/v2/
until it becomes healthy or a configurable timeout is reached: use LOG_FILE for
logging, attempt curl repeatedly (e.g., every 2s with a maximum timeout or
exponential backoff), print progress messages to tee -a $LOG_FILE, and if the
timeout is exceeded return 1; update the block that currently references
REGISTRY_HOST, REGISTRY_PORT, and LOG_FILE to implement this loop and timeout
behavior instead of the single sleep and one-off curl.
- Around line 257-264: The verification pull should use the explicit tag that
was pushed rather than hardcoding :latest; update the test_image construction
(currently using
test_image="$REGISTRY_HOST:$REGISTRY_PORT/mattermost/calls-recorder:latest") to
reference the version/tag variable used when tagging/pushing (e.g., IMAGE_TAG or
PUSH_TAG), falling back to :latest only if that variable is empty, and keep
logging to $LOG_FILE; this ensures the docker pull verifies the exact image that
was pushed.
- Around line 153-158: Quote shell variables to prevent word splitting and
globbing: update the docker pull and docker tag commands to use
"$image:$version" and "$local_tag" and wrap redirections using "$LOG_FILE";
build the local_tag using quoted/brace-expanded variables (e.g.,
local_tag="${REGISTRY_HOST}:${REGISTRY_PORT}/${image}:${version}") so that
REGISTRY_HOST, REGISTRY_PORT, image, and version are safe when they contain
spaces or special chars; ensure all echo/tee and docker invocations reference
"$local_tag" and "$LOG_FILE" consistently.
In `@source/administration-guide/configure/calls-offloader-setup.md`:
- Around line 796-851: The embedded script update-air-gap-images.sh currently
hardcodes NEW_RECORDER_VERSION ("v0.8.5") and NEW_TRANSCRIBER_VERSION
("v0.6.3"); change it to accept these as parameters or environment variables and
update the documentation to call out that users must set NEW_RECORDER_VERSION
and NEW_TRANSCRIBER_VERSION (or export REGISTRY_HOST/REGISTRY_PORT) before
running the script; specifically, modify the script template to read versions
from $1/$2 or from ${NEW_RECORDER_VERSION:-} and ${NEW_TRANSCRIBER_VERSION:-}
and add a prominent note in the docs explaining how to supply or override these
values prior to running update-air-gap-images.sh.
- Around line 673-677: Change the sentence that reads "Ensure both versioned
tags (e.g., `v0.8.5`) and `latest` tags are present in your local registry" to
include the missing word "that" so it reads "Ensure that both versioned tags
(e.g., `v0.8.5`) and `latest` tags are present in your local registry"; update
this in the Calls offloader setup section of configure/calls-offloader-setup.md
where the bullet "Ensure both versioned tags..." appears.
- Line 734: The phrase "On internet-connected machine" is missing the article;
update the sentence to "On an internet-connected machine, run the air-gap setup
script with new versions:" so the text string "On internet-connected machine"
becomes "On an internet-connected machine" near the "air-gap setup script"
instruction.
In `@source/administration-guide/configure/calls-rtcd-setup.md`:
- Around line 254-261: The TOML snippet under the [rtc] section has inconsistent
indentation for the ice_servers array; remove the extra leading whitespace so
the ice_servers key and its value are aligned with the section header (i.e.,
make the ice_servers line and its array entries have consistent indentation
under [rtc]), ensuring the identifier ice_servers and the [rtc] block remain
unchanged and the array items (urls/username/credential) are indented
consistently.
- Line 108: The sentence "This is the recommended deployment method for
non-Kubernetes production environments as it provides the best performance and
operational control." needs a comma after "environments" to improve readability;
update the sentence in configure/calls-rtcd-setup.md (the line containing that
sentence) to read "...for non-Kubernetes production environments, as it provides
the best performance and operational control."
- Around line 19-45: The inline CSS for table styling (selectors like
table.network-requirements, table.network-requirements th/td, and
body:not([data-custom-theme="light"]) table.network-requirements ...) should be
removed from the Markdown and moved into the project's central/shared stylesheet
or a documentation CSS include; create or add these rules to the shared CSS
module used by docs, update any import/includes so pages using the
network-requirements table get the styles, and delete the embedded <style> block
from configure/calls-rtcd-setup.md to avoid duplicate definitions and ease
maintenance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c67fe115-882c-445d-9e22-693000c33df1
📒 Files selected for processing (3)
scripts/air-gap-docker-registry-setup.shsource/administration-guide/configure/calls-offloader-setup.mdsource/administration-guide/configure/calls-rtcd-setup.md
Contributor
Author
|
Hi @bgardner8008 - Would you have some time to finalize this review. I'd love to get these changes published before my contract ends on Friday. |
bgardner8008
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA ae36560 |
|
Calls Deployment Overview: LGTM, though I note the section title doesn't match the index "Deploy Mattermost Calls". RTCD Setup and Configuration: "For detailed capability/credential configuration, reference the RTCD Service URL documentation." I think you could just remove this sentence as the rtcd-service-url section doesn't appear to exist. Calls Offloader Setup and Configuration: "Additionally, this override configuration is particularly useful if your Mattermost public Site URL uses HTTPS, because pointing the offloader directly to an internal HTTP endpoint bypasses the need to deal with client-side certificates." This is incorrect. Using http instead of https lets the offloader connect to the server without worrying about the server's TLS certificate. It has nothing to do with client side certs. You could simply state: "This override configuration lets the recorder and transcriber jobs connect to mattermost server using HTTP instead of HTTPS, which should only be used in a private network." Offloader Air-gapped installation There's a LOT of content devoted to setting up air-gapped deployment, which is all pretty moot given my script will do it all. And furthermore this section still references the older scripts which don't work correctly. I think we should just go all in on using the new script, it's 1000x easier. I will send you a .md file with complete rewrite for the "Air-Gapped Installation of calls-offloader" section. Calls Metrics and Monitoring: LGTM Kubernetes: Calls Deployment on Kubernetes The diagram shows RTCD running behind a load balancer. That's not actually correct as I've discussed earlier. If you have the ability to edit the image, the incorrect bit is the blue load-balancer icon in the RTCD cluster receiving connections from the mattermost cluster, please remove. If you can't edit the image, then just leave it. In the description of the image, remove item 2: "RTCD services are exposed through load balancers" In Kubernetes-Specific Configuration -> Network Configuration, remove item 3 "Load balancer configuration: Load balancers must be properly configured to handle UDP and TCP traffic routing to RTCD pods". In Scaling Considerations, Item 2 is incorrect and should be changed to: "DNS A records should be set up for all RTCD pods so server can discover RTCD servers." Item 3 should be: Health checks should ensure that only healthy pods are reported via DNS. Troubleshooting: LGTM |
- calls-offloader-setup: remove large sections of content per review - calls-kubernetes: remove inaccurate load balancer references, clarify DNS-based RTCD discovery - calls-deployment, calls-rtcd-setup: minor corrections Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/administration-guide/configure/calls-kubernetes.md (1)
111-111:⚠️ Potential issue | 🔴 CriticalRemove the orphaned numbered list item.
Line 111 contains a stray "3." list item that doesn't belong in the "Other Calls Documentation" section. This same troubleshooting link already appears correctly in the bullet list at line 110. The orphaned item breaks the document structure.
🗑️ Proposed fix to remove the duplicate
- [Calls Metrics and Monitoring](calls-metrics-monitoring.md): Guide to monitoring Calls performance using metrics and observability - [Calls Troubleshooting](calls-troubleshooting.md): Detailed troubleshooting steps and debugging techniques -3. If you encounter issues, see [Calls Troubleshooting](calls-troubleshooting.md)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/administration-guide/configure/calls-kubernetes.md` at line 111, Remove the stray numbered list item "3. If you encounter issues, see [Calls Troubleshooting](calls-troubleshooting.md)" so the duplicate troubleshooting link is gone; locate the orphaned "3." entry in the "Other Calls Documentation" section and delete that single line (the correctly placed bullet at the previous line should remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/administration-guide/configure/calls-kubernetes.md`:
- Line 111: Remove the stray numbered list item "3. If you encounter issues, see
[Calls Troubleshooting](calls-troubleshooting.md)" so the duplicate
troubleshooting link is gone; locate the orphaned "3." entry in the "Other Calls
Documentation" section and delete that single line (the correctly placed bullet
at the previous line should remain unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c97b9271-d575-4b11-8500-7aad097cd62d
📒 Files selected for processing (4)
source/administration-guide/configure/calls-deployment.mdsource/administration-guide/configure/calls-kubernetes.mdsource/administration-guide/configure/calls-offloader-setup.mdsource/administration-guide/configure/calls-rtcd-setup.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
bgardner8008
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA 5632854 |
|
When the air-gap-offloader documentation was imported, all the headings came in one level too high. So for example that section now appears as a separate chapter index in left hand navigation. You'll need to drop the headings down one level and it should be good. |
…ation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
Ah shoot, good catch @bgardner8008 ! A fix should be there now, and the one item from coderabbit. |
bgardner8008
added
preview-environment
Contributor
|
Newest code from sadohert has been published to preview environment for Git SHA ad52a65 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sub pages were added. Claude seemed to expand on the documentation for calls configuration, including deployment, metrics monitoring, RTC setup, and troubleshooting.
Summary by CodeRabbit