feat: support custom OLR image in tests + fix Unicode handling#25
Merged
Conversation
📝 WalkthroughWalkthroughThis PR parameterizes Docker container execution across multiple test environments. Docker Compose files use OLR_IMAGE, OLR_UID, and OLR_GID environment variables for flexible image and user configuration. Supporting scripts export these values, test fixtures apply them, and Docker exec operations ensure Oracle locale consistency. ChangesContainer user and environment parameterization
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/dbz-twin/docker-compose.yaml (1)
45-45:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHardcoded tmpfs UID conflicts with configurable OLR_UID.
The tmpfs mount specifies
uid=1000, but the service now runs as${OLR_UID:-1000}. When a customOLR_UIDis set (e.g., for release images running as UID 1001), the container process won't have write permissions to/olr-data.Docker Compose doesn't support variable substitution in tmpfs options. Consider one of these approaches:
- Document that
OLR_UIDmust remain 1000 for the dbz-twin environment- Use a named volume instead of tmpfs
- Add an init script to chown the tmpfs mount at container startup
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dbz-twin/docker-compose.yaml` at line 45, The tmpfs mount in tests/dbz-twin/docker-compose.yaml hardcodes uid=1000 which conflicts with the configurable OLR_UID; update the compose setup to avoid this mismatch by either switching the tmpfs to a named volume (replace the tmpfs entry with a named volume mount) or add an init step inside the container startup (e.g., an entrypoint/init script referenced by the service) that chowns /olr-data to ${OLR_UID:-1000} at runtime; ensure references to OLR_UID remain consistent and document the chosen approach in the compose/service comments so the service (which currently expects ${OLR_UID:-1000}) can write to /olr-data.
🧹 Nitpick comments (1)
tests/sql/scripts/drivers/base.sh (1)
37-38: 💤 Low valueShellcheck warns about potentially masking command failures.
Combining declaration and assignment masks the exit status of
id -uandid -g. While these commands rarely fail, splitting the operations would be more robust.♻️ Proposed fix to satisfy shellcheck
# Export UID/GID so docker-compose can set user: on the olr service -export OLR_UID="$(id -u)" -export OLR_GID="$(id -g)" +OLR_UID="$(id -u)" +OLR_GID="$(id -g)" +export OLR_UID OLR_GID🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sql/scripts/drivers/base.sh` around lines 37 - 38, The assignments to OLR_UID and OLR_GID mask failures of id -u / id -g; change to run id -u and id -g separately, check their exit status, and only export OLR_UID and OLR_GID after confirming the commands succeeded (e.g., capture each output into a temp variable, verify $? is zero, then export OLR_UID and OLR_GID). Update the script locations where OLR_UID and OLR_GID are set (the lines exporting OLR_UID and OLR_GID in base.sh) to follow this safer pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/dbz-twin/docker-compose.yaml`:
- Line 45: The tmpfs mount in tests/dbz-twin/docker-compose.yaml hardcodes
uid=1000 which conflicts with the configurable OLR_UID; update the compose setup
to avoid this mismatch by either switching the tmpfs to a named volume (replace
the tmpfs entry with a named volume mount) or add an init step inside the
container startup (e.g., an entrypoint/init script referenced by the service)
that chowns /olr-data to ${OLR_UID:-1000} at runtime; ensure references to
OLR_UID remain consistent and document the chosen approach in the
compose/service comments so the service (which currently expects
${OLR_UID:-1000}) can write to /olr-data.
---
Nitpick comments:
In `@tests/sql/scripts/drivers/base.sh`:
- Around line 37-38: The assignments to OLR_UID and OLR_GID mask failures of id
-u / id -g; change to run id -u and id -g separately, check their exit status,
and only export OLR_UID and OLR_GID after confirming the commands succeeded
(e.g., capture each output into a temp variable, verify $? is zero, then export
OLR_UID and OLR_GID). Update the script locations where OLR_UID and OLR_GID are
set (the lines exporting OLR_UID and OLR_GID in base.sh) to follow this safer
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ccdce2b5-761d-46e3-86f1-76625d9e487e
📒 Files selected for processing (8)
tests/dbz-twin/docker-compose.yamltests/environments/enterprise-19/docker-compose.yamltests/environments/free-23/docker-compose.yamltests/environments/xe-21-official/docker-compose.yamltests/environments/xe-21/docker-compose.yamltests/fixtures/test_fixtures.pytests/sql/scripts/drivers/base.shtests/sql/scripts/drivers/docker.sh
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
1 participant
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.
Summary
OLR_IMAGEenv var in all docker-compose files (enables testing release images likeghcr.io/rophy/openlogreplicator:v1.9.0.1)--userto docker run in redo log tests for release image compatibility (release image runs as UID 1001)user:override in compose services, exported from base driverNLS_LANG=AMERICAN_AMERICA.AL32UTF8in docker exec calls — enterprise-19 defaulted to US7ASCII, corrupting Unicode in LogMiner outputTest plan
Summary by CodeRabbit