feat(validations): Add early required validation#5166
feat(validations): Add early required validation#5166AdarshK15 wants to merge 10 commits intoGoogleCloudPlatform:developfrom
Conversation
Summary of ChangesHello @AdarshK15, 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 module-scoped validation capabilities by introducing a versatile 'required' validator. This new validator allows module authors to define precise constraints on input variables within their Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable required validator, which is effectively used across several modules to enforce required settings and deprecate old ones. The implementation is solid, but I've identified a couple of logic issues in the new helper functions that could lead to incorrect validation behavior in edge cases. Specifically, the logic for determining if a variable is 'set' needs refinement for empty collections and numeric zero values. I've also noted a minor formatting issue in an error message. The accompanying documentation and test cases are a great start.
b237cb4 to
979ef5d
Compare
979ef5d to
ed8533b
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable required validator, enhancing the module validation framework. The implementation is solid, with comprehensive tests and clear documentation. I've suggested one refactoring in pkg/validators/metadata_validators.go to improve code consistency and maintainability by leveraging an existing helper function, which aligns with our goal for technical excellence and maintainable code.
Summary
This PR introduces the required validator to the module-scoped validation framework. It allows enforcing the presence of mandatory settings or explicitly forbid specific settings (e.g., for deprecation) directly within metadata.yaml. By moving these checks into the metadata layer, these constraints are validated earlier in the deployment lifecycle.
Key Changes
New RequiredValidator: Implements a validator to enforce the presence or absence of module settings.
Functionality:
useful for preventing the use of deprecated variables or enforcing "must not be set" constraints.
non-empty collection (list/map).
Testing
Added unit tests in pkg/validators/metadata_validators_test.go covering:
Documentation
Updated docs/blueprint-validation.md to include usage instructions and YAML examples for both enforcing required
variables and forbidding unwanted ones.