Skip to content

Conversation

mhamza15
Copy link

@mhamza15 mhamza15 commented Apr 7, 2025

…ableACLConfig is true (vitessio#17274)

Backports vitessio#17274

Closes https://github.com/github/vitess/issues/1338

…ableACLConfig is true (vitessio#17274)

Signed-off-by: garfthoffman <[email protected]>
Signed-off-by: Mohamed Hamza <[email protected]>
Co-authored-by: Mohamed Hamza <[email protected]>
@mhamza15 mhamza15 self-assigned this Apr 7, 2025
@Copilot Copilot AI review requested due to automatic review settings April 7, 2025 14:51
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures that loading an ACL configuration fails when the provided file is empty, preventing further processing on an invalid config.

  • Added a check in tableacl.go to detect empty configuration data and return an error.
  • Added a new test, TestInitWithEmptyConfig, in tableacl_test.go to validate that an empty file triggers the expected error.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
go/vt/tableacl/tableacl.go Inserts a block to check for empty configuration file content and returns an error if empty.
go/vt/tableacl/tableacl_test.go Introduces a test to ensure the system fails initialization when an empty config file is provided.
Comments suppressed due to low confidence (1)

go/vt/tableacl/tableacl_test.go:88

  • Consider verifying that the error message contains the expected text 'tableACL config file is empty' to ensure that the error is specifically raised due to the empty file.
require.Error(t, err)

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@arthurschreiber arthurschreiber merged commit fea1028 into release-19.0-github Apr 7, 2025
195 of 201 checks passed
@mhamza15 mhamza15 deleted the mhamza/backport-empty-acl-fix branch April 7, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants