Conversation
…tests added odbc and mysql dev deps
Review Summary by QodoAdd database support and expand test coverage
WalkthroughsDescription• Add locale configuration and ODBC/MySQL database support • Simplify APT preferences by removing stable pin priority • Add database clients and development packages (PostgreSQL, MariaDB, ODBC, FreeTDS) • Expand test coverage with new command tests for database and utility tools Diagramflowchart LR
A["Dockerfile"] -->|"Add locales config"| B["UTF-8 Environment"]
A -->|"Add DB packages"| C["PostgreSQL, MariaDB, ODBC, FreeTDS"]
A -->|"Simplify APT"| D["Deprioritize testing repo"]
E["specs.yaml"] -->|"Add DB tests"| F["PostgreSQL, ODBC, FreeTDS"]
E -->|"Add tool tests"| G["Node.js, wget, curl, jq, java"]
File Changes1. Dockerfile
|
Summary of ChangesHello @jhassine, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the development environment within the Docker image by integrating essential tools and libraries for improved database connectivity (MySQL, ODBC), general build processes, and common utility commands. It also introduces robust testing to ensure these new additions are correctly installed and functional, thereby improving the reliability and completeness of the environment. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully enhances the Dockerfile with locale support, additional database development dependencies (MariaDB/MySQL and ODBC), and common build tools. It also adds comprehensive tests for these new components. I have identified a few opportunities for improvement regarding build performance consistency, package compatibility, and test deduplication.
There was a problem hiding this comment.
Pull request overview
This PR enhances the base Docker image by adding locale configuration and additional tooling/development dependencies (ODBC, MariaDB/MySQL dev libs, extra CLI tools), and extends the container structure tests to validate the new capabilities.
Changes:
- Configure
en_US.UTF-8locales and setLC_ALLin the Docker image. - Add additional apt packages (e.g.,
pkg-config,wget, PostgreSQL client, MariaDB dev libs, unixODBC/FreeTDS). - Expand
tests/specs.yamlcommand tests to assert availability of the newly added tools/features.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
Dockerfile |
Adds locale setup and installs additional build tools, DB/ODBC dependencies, and CLI utilities. |
tests/specs.yaml |
Adds command-based assertions for PostgreSQL, Node.js, ODBC/FreeTDS, wget/curl/jq, and Java. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: "nodejs is installed in path" | ||
| command: "node" | ||
| args: ["--version"] | ||
| expectedOutput: ["v2"] # Node 20+ outputs "v2x.x.x" |
There was a problem hiding this comment.
The Node.js version assertion expectedOutput: ["v2"] effectively requires Node 20+ (v20/v22/...). The Dockerfile installs nodejs without pinning a major version, so this test may fail if the base image repo provides Node 18 (v18...). Either relax the assertion (e.g., just check that node --version prints a v) or explicitly pin/install Node 20+ in the Dockerfile to match this test.
| expectedOutput: ["v2"] # Node 20+ outputs "v2x.x.x" | |
| expectedOutput: ["v"] # Node outputs "vX.Y.Z"; just confirm a version string is printed |
Dockerfile
Outdated
| libmariadb-dev \ | ||
| # For ODBC support: | ||
| unixodbc-dev \ | ||
| freetds-dev tdsodbc \ |
There was a problem hiding this comment.
Package list formatting: freetds-dev tdsodbc puts two packages on one line while the rest are one-per-line. Splitting them improves readability and reduces diff churn when adding/removing packages later.
| freetds-dev tdsodbc \ | |
| freetds-dev \ | |
| tdsodbc \ |
Code Review by Qodo
1. Node version not guaranteed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| - name: "nodejs is installed in path" | ||
| command: "node" | ||
| args: ["--version"] | ||
| expectedOutput: ["v2"] # Node 20+ outputs "v2x.x.x" |
There was a problem hiding this comment.
1. Node version not guaranteed 🐞 Bug ✓ Correctness
The structure test requires Node’s version string to start with "v2" (Node 20+), but the Dockerfile installs the distro "nodejs" package without pinning/enforcing a 20+ major. This can break CI if the base repo provides Node 18/19 while still correctly installing Node.
Agent Prompt
### Issue description
`tests/specs.yaml` asserts `node --version` starts with `v2` (Node 20+), but the Dockerfile installs `nodejs` without any version constraint. This makes the test brittle across base-image/repo changes.
### Issue Context
The image installs `nodejs` from the default apt repos in the main install layer. The test comment explicitly encodes a Node 20+ assumption.
### Fix Focus Areas
- tests/specs.yaml[118-121]
- Dockerfile[78-86]
### Suggested fixes
Choose one:
1) **Enforce Node 20+** in the Docker build (e.g., install `nodejs` from the same temporary `testing` enablement step with `-t testing`, or pin the package version explicitly).
2) **Relax the structure test** to something like `expectedOutput: ["v"]` or a regex matching any semver (depending on container-structure-test semantics), so the test validates presence rather than a specific major.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: "freetds support is available" | ||
| command: "odbcinst" | ||
| args: ["-q", "-d", "-n", "FreeTDS"] | ||
| expectedOutput: ["FreeTDS"] |
There was a problem hiding this comment.
2. Freetds test is brittle 🐞 Bug ⛯ Reliability
The new test assumes an ODBC driver entry exists with the exact name "FreeTDS", but the Dockerfile doesn’t explicitly register/configure that driver name anywhere. This can cause CI failures if the package registers a different name or doesn’t populate /etc/odbcinst.ini in this environment.
Agent Prompt
### Issue description
The structure test hard-codes an ODBC driver lookup by name (`FreeTDS`). The Dockerfile installs packages but does not explicitly ensure an `odbcinst` driver entry with that exact name exists.
### Issue Context
Package post-install scripts may or may not register the driver under the expected name; relying on this implicitly makes CI flaky across distro/package variations.
### Fix Focus Areas
- tests/specs.yaml[128-131]
- Dockerfile[96-101]
### Suggested fixes
Pick one:
1) **Explicitly register/configure** the FreeTDS driver during build (e.g., write a deterministic `/etc/odbcinst.ini` entry or invoke `odbcinst` install commands).
2) **Relax the test** to verify FreeTDS support in a less name-coupled way, e.g. `odbcinst -q -d` and assert the output contains `FreeTDS` (or check the presence of the `tdsodbc` shared object) rather than `-n FreeTDS`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
phuongfi91
left a comment
phuongfi91
left a comment
There was a problem hiding this comment.
One small comment otherwise looking good 😎 👍🏿
…ing obsolete psql test
…tests
added odbc and mysql dev deps