Minor Upstream Update of v21 #197
Conversation
* fix actions Signed-off-by: Mohamed Hamza <[email protected]> * fix actions Signed-off-by: Mohamed Hamza <[email protected]> --------- Signed-off-by: Mohamed Hamza <[email protected]>
#176) * Fail loading an ACL config if the provided file is empty and enforceTableACLConfig is true (vitessio#17274) Signed-off-by: garfthoffman <[email protected]> Signed-off-by: Mohamed Hamza <[email protected]> Co-authored-by: Mohamed Hamza <[email protected]> * rerun ci --------- Signed-off-by: garfthoffman <[email protected]> Signed-off-by: Mohamed Hamza <[email protected]> Co-authored-by: garfthoffman <[email protected]>
* Set parsed comments in operator for subqueries (vitessio#18369) Signed-off-by: David Piegza <[email protected]> * rerun ci Signed-off-by: Mohamed Hamza <[email protected]> * remove skip_e2e Signed-off-by: Mohamed Hamza <[email protected]> * fix test cases --------- Signed-off-by: David Piegza <[email protected]> Signed-off-by: Mohamed Hamza <[email protected]> Co-authored-by: David Piegza <[email protected]>
…ig (vitessio#17984) (#190) Signed-off-by: Florent Poinsard <[email protected]> Signed-off-by: Mohamed Hamza <[email protected]> Co-authored-by: Florent Poinsard <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces conditional GitHub Actions runner selection and implements password retrieval from credentials files for unmanaged tablets. It also fixes a bug where SQL comments were not properly preserved when subqueries are merged, and adds validation for empty tableACL configuration files.
- Conditional runner selection for GitHub Actions workflows to use custom 16-core runners only for the
vitessioorganization - Support for retrieving database passwords from credentials files for unmanaged tablets
- Preservation of SQL comments when subqueries are merged into a single route
- Validation to prevent empty tableACL configuration files from being loaded
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/templates/*.tpl | Updates template files to conditionally use 16-core runners based on repository owner |
| .github/workflows/*.yml | Updates generated workflow files with conditional runner selection logic |
| go/vt/vttablet/tabletserver/tabletenv/config.go | Adds fallback logic to retrieve password from credentials server when not directly specified |
| go/vt/vttablet/tabletserver/tabletenv/config_test.go | Adds test coverage for password retrieval from credentials file |
| go/vt/vttablet/tabletserver/tabletenv/data/db_credentials.json | Adds test credentials file |
| go/vt/dbconfigs/credentials.go | Adds test helper function to set credentials file path |
| go/vt/vtgate/planbuilder/operators/subquery_planning.go | Preserves comments when merging subqueries with outer queries |
| go/vt/vtgate/planbuilder/testdata/select_cases.json | Adds test cases for comment preservation in merged and unmerged subqueries |
| go/vt/tableacl/tableacl.go | Adds validation to reject empty tableACL configuration files |
| go/vt/tableacl/tableacl_test.go | Adds test for empty tableACL configuration file handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, pass, err := dbconfigs.GetCredentialsServer().GetUserAndPassword(c.DB.App.User) | ||
| if err == nil && pass != "" { | ||
| c.DB.App.Password = pass | ||
| } else { | ||
| return errors.New("database app user password not specified") | ||
| } |
There was a problem hiding this comment.
The error message 'database app user password not specified' is misleading when a credentials server is configured but fails to return credentials. Consider providing a more specific error message that distinguishes between the password not being set and the credentials server failing to retrieve it.
| return cs | ||
| } | ||
|
|
||
| // SetDbCredentialsFilePath sets the value of the `--db-credentials-file` flag. |
There was a problem hiding this comment.
[nitpick] The comment uses backticks around the flag name, but the actual flag is '--db-credentials-file' (with double dashes). The backticks should be removed for consistency with standard Go comment conventions, or the entire flag reference should use proper markdown formatting.
| // SetDbCredentialsFilePath sets the value of the `--db-credentials-file` flag. | |
| // SetDbCredentialsFilePath sets the value of the --db-credentials-file flag. |
Description
Related Issue(s)
Checklist
Deployment Notes