-
Notifications
You must be signed in to change notification settings - Fork 2k
proof-of-concept: chore: migrate knex to new ci #4054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @glasnt, 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!
Summary of Changes
This pull request migrates the knex configuration to the new CI setup, using the branch from PR #4040 as its base. The changes involve updating the nodejs-dev.jsonc and nodejs-prod.jsonc files to include or exclude specific cloud-sql configurations. Additionally, it introduces new ci-setup.json files for both mysql and postgres knex configurations, and modifies the package.json files to include a proxy script for running system tests.
Highlights
- CI Migration: The primary goal is to migrate the knex configuration to the new CI environment.
- Configuration Updates: The
nodejs-dev.jsoncandnodejs-prod.jsoncfiles are updated to reflect the new CI setup, including enabling cloud-sql configurations in dev and disabling them in prod. - New CI Setup Files: New
ci-setup.jsonfiles are added for both mysql and postgres knex, defining environment variables and secrets required for CI. - Proxy Script Integration: The
package.jsonfiles are modified to include a proxy script for running system tests, ensuring proper connectivity during testing.
Changelog
Click here to see the changelog
- .github/config/nodejs-dev.jsonc
- Added
cloud-sql/mysql/mysqlandcloud-sql/postgres/knexto the list of included configurations.
- Added
- .github/config/nodejs-prod.jsonc
- Removed
cloud-sql/mysql/mysqlandcloud-sql/postgres/knexfrom the list of included configurations.
- Removed
- cloud-sql/mysql/mysql/ci-setup.json
- Created a new
ci-setup.jsonfile to define environment variables (INSTANCE_HOST, INSTANCE_CONNECTION_NAME, UNIX_SOCKET_DIR, CLOUD_SQL_CONNECTION_NAME, INSTANCE_UNIX_SOCKET, DB_NAME, DB_USER) and secrets (DB_PASS) for the MySQL CI setup. - Defines the environment and secrets needed for the mysql cloud sql instance to run in CI.
- Created a new
- cloud-sql/mysql/mysql/package.json
- Added a
proxyscript that executessql-proxy.sh. - Modified
system-testandsystem-test-unixscripts to use theproxyscript before running tests.
- Added a
- cloud-sql/postgres/knex/ci-setup.json
- Created a new
ci-setup.jsonfile to define environment variables (INSTANCE_HOST, INSTANCE_CONNECTION_NAME, UNIX_SOCKET_DIR, CLOUD_SQL_CONNECTION_NAME, INSTANCE_UNIX_SOCKET, DB_NAME, DB_USER) and secrets (DB_PASS) for the PostgreSQL CI setup. - Defines the environment and secrets needed for the postgres knex cloud sql instance to run in CI.
- Created a new
- cloud-sql/postgres/knex/package.json
- Added a
proxyscript that executesdebug-sql-proxy.sh. - Modified the
testscript to use theproxyscript before running tests.
- Added a
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
In the realm of CI, a migration's call,
Knex and Cloud SQL, standing tall.
With proxies and scripts, a testing spree,
Ensuring the database flows free.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request migrates Knex to the new CI. The changes involve updating configuration files and package.json files for both MySQL and PostgreSQL with Knex. The addition of ci-setup.json files and modifications to the package.json scripts are the main focus. The goal is to integrate these projects into the new CI environment.
Summary of Findings
- Missing tests in production configuration: The
cloud-sql/mysql/mysqlandcloud-sql/postgres/knexentries were removed from thenodejs-prod.jsoncfile, but there is no clear indication of whether the underlying issues have been resolved. Removing them without fixing the underlying issues might lead to regressions in production. - Proxy script consistency: The
cloud-sql/mysql/mysqlandcloud-sql/postgres/knexprojects use different proxy scripts (sql-proxy.shvsdebug-sql-proxy.sh). It's important to ensure consistency in the proxy scripts used across different projects to avoid unexpected behavior or configuration discrepancies.
Merge Readiness
The pull request introduces changes to integrate Knex into the new CI. However, there are a couple of points that need to be addressed before merging. The removal of entries from the production configuration without clear resolution of the underlying issues and the inconsistency in proxy scripts should be investigated. I am unable to directly approve this pull request, and recommend that these points be addressed before merging. It is also recommended that others review and approve this code before merging.
| "automl", // (untested) FAILED_PRECONDITION: Google Cloud AutoML Natural Language was retired on March 15, 2024. Please migrate to Vertex AI instead | ||
| "cloud-sql/mysql/mysql", // (untested) Error: expected 200 "OK", got 500 "Internal Server Error" | ||
| "cloud-sql/mysql/mysql2", // (untested) Error: Cannot find module './connect-connector-with-iam-authn.js' | ||
| "cloud-sql/postgres/knex", // (untested) CloudSQLConnectorError: Malformed instance connection name provided: expected format of "PROJECT:REGION:INSTANCE", got undefined | ||
| "cloud-sql/sqlserver/mssql", // (untested) TypeError: The "config.server" property is required and must be of type string. | ||
| "cloud-sql/sqlserver/tedious", // (untested) TypeError: The "config.server" property is required and must be of type string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "proxy": "$GITHUB_WORKSPACE/.github/workflows/utils/sql-proxy.sh", | ||
| "system-test": "npm run proxy -- c8 mocha -p -j 2 test/server.test.js --colors --timeout=60000 --exit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
this PR was successful, the failure was only due to conventional commits |
Description
Migrates knex into new CI, using #4040 as the base branch
Do not merge