Minor Upstream Update of v21 #197
Closed
andyedison wants to merge 4 commits intorelease-21.0from
Closed
Conversation
* fix actions Signed-off-by: Mohamed Hamza <mhamza15@github.com> * fix actions Signed-off-by: Mohamed Hamza <mhamza15@github.com> --------- Signed-off-by: Mohamed Hamza <mhamza15@github.com>
#176) * Fail loading an ACL config if the provided file is empty and enforceTableACLConfig is true (vitessio#17274) Signed-off-by: garfthoffman <109185460+garfthoffman@users.noreply.github.com> Signed-off-by: Mohamed Hamza <mhamza15@github.com> Co-authored-by: Mohamed Hamza <mhamza15@github.com> * rerun ci --------- Signed-off-by: garfthoffman <109185460+garfthoffman@users.noreply.github.com> Signed-off-by: Mohamed Hamza <mhamza15@github.com> Co-authored-by: garfthoffman <109185460+garfthoffman@users.noreply.github.com>
* Set parsed comments in operator for subqueries (vitessio#18369) Signed-off-by: David Piegza <davidpiegza@github.com> * rerun ci Signed-off-by: Mohamed Hamza <mhamza15@github.com> * remove skip_e2e Signed-off-by: Mohamed Hamza <mhamza15@github.com> * fix test cases --------- Signed-off-by: David Piegza <davidpiegza@github.com> Signed-off-by: Mohamed Hamza <mhamza15@github.com> Co-authored-by: David Piegza <697113+davidpiegza@users.noreply.github.com>
…ig (vitessio#17984) (#190) Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr> Signed-off-by: Mohamed Hamza <mhamza15@github.com> Co-authored-by: Florent Poinsard <35779988+frouioui@users.noreply.github.com>
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.
Comment on lines
+880
to
+885
| _, 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.
Suggested change
| // SetDbCredentialsFilePath sets the value of the `--db-credentials-file` flag. | |
| // SetDbCredentialsFilePath sets the value of the --db-credentials-file flag. |
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
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.
Description
Related Issue(s)
Checklist
Deployment Notes