Skip to content

Conversation

@rswanson
Copy link
Member

No description provided.

@rswanson rswanson mentioned this pull request May 27, 2025
Copy link
Contributor

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 updates validation logic and associated tests for multiple component arguments to ensure consistent error messages and required field checks across SignetNode, Builder, and Quincey components.

  • Updated SignetNodeComponentArgs and SignetNodeEnv validations and tests with additional required fields and error messages.
  • Revised BuilderEnv validations and adjusted test expectations to accommodate newly added required fields.
  • Added tests for QuinceyComponentArgs validations to verify proper error reporting.

Reviewed Changes

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

Show a summary per file
File Description
pkg/signet_node/validation_test.go Updated tests to check new required field validations and messages.
pkg/signet_node/validation.go Modified required field checks and error messages for SignetNode.
pkg/quincey/validation_test.go Added comprehensive validations for Quincey component arguments.
pkg/builder/validation_test.go Adjusted test expectations for BuilderEnv validations.
pkg/builder/validation.go Expanded required field validations for BuilderEnv with new fields.
pkg/builder/types.go Added validation tags for required fields in BuilderEnv.
Comments suppressed due to low confidence (1)

pkg/builder/validation_test.go:66

  • The expected error message was changed from 'builder port is required' to 'auth token refresh interval is required'. Please double-check that this aligns with the intended validation logic for BuilderEnv.
assert.Equal(t, "auth token refresh interval is required", err.Error())

Copy link
Contributor

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@rswanson rswanson self-assigned this May 28, 2025
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from 38ca39b to cbf4d47 Compare May 29, 2025 11:25
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from cbf4d47 to 90c4115 Compare May 29, 2025 11:29
@rswanson rswanson requested a review from Copilot May 29, 2025 11:32
Copy link
Contributor

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 adds comprehensive validation checks for newly introduced fields in the SignetNode, Builder, and Quincey components, and updates related tests and struct tags to enforce required inputs.

  • Expanded Validate() methods to cover all new Pulumi inputs in SignetNodeComponentArgs, SignetNodeEnv, and BuilderEnv.
  • Updated unit tests to reflect new validation rules and removed older env-specific test suites.
  • Added validate:"required" tags to struct definitions in types.go for automatic schema validation.

Reviewed Changes

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

Show a summary per file
File Description
pkg/signet_node/validation.go Added nil-checks for new SignetNodeComponentArgs and SignetNodeEnv fields
pkg/signet_node/types.go Annotated SignetNodeEnv fields with validate:"required"
pkg/signet_node/validation_test.go Adjusted tests to cover validation of added SignetNode fields, removed old env tests
pkg/quincey/validation_test.go Created tests for QuinceyComponentArgs validation scenarios
pkg/builder/validation.go Extended BuilderEnv.Validate() to require all newly added fields
pkg/builder/types.go Added validate:"required" tags on BuilderEnv fields
pkg/builder/validation_test.go Updated builder component args tests to align with reordered validation
Comments suppressed due to low confidence (4)

pkg/signet_node/validation_test.go:68

  • The test invalidArgs1 provides Namespace but omits Name, so Validate() returns "name is required" rather than "namespace is required". Update the assertion to expect "name is required".
assert.Contains(t, err.Error(), "namespace is required")

pkg/signet_node/validation_test.go:104

  • [nitpick] The detailed per-field tests for SignetNodeEnv.Validate() were removed. Consider adding targeted tests for each required env field to ensure all error paths are covered.
// Test with invalid env

pkg/signet_node/validation.go:46

  • [nitpick] Error messages use lowercase "url" here but uppercase acronyms elsewhere (e.g., "RPC URL"). Standardize the casing of acronyms in all error messages for consistency.
if env.BlobExplorerUrl == nil {

pkg/quincey/validation_test.go:70

  • [nitpick] The test only checks a high-level error for an invalid QuinceyEnv. Add tests for individual missing QuinceyEnv fields to verify each validation message.
assert.Contains(t, err.Error(), "env is invalid")

@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from 90c4115 to 5d9b2c5 Compare May 29, 2025 13:54
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from 5d9b2c5 to c1953db Compare May 29, 2025 13:55
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from c1953db to 7d811db Compare May 29, 2025 15:24
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from 7d811db to caac358 Compare May 29, 2025 15:30
@rswanson rswanson requested review from Evalir and dylanlott May 29, 2025 15:58
@rswanson rswanson force-pushed the swanny/dry branch 2 times, most recently from 31bd4dc to 17efe1f Compare June 3, 2025 14:38
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch 2 times, most recently from 0a084a5 to bd6d961 Compare June 3, 2025 14:38
@rswanson rswanson changed the base branch from swanny/dry to graphite-base/10 June 3, 2025 14:39
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from bd6d961 to af1fc29 Compare June 3, 2025 14:39
@rswanson rswanson force-pushed the graphite-base/10 branch from ec22259 to 4544404 Compare June 3, 2025 14:39
@graphite-app graphite-app bot changed the base branch from graphite-base/10 to main June 3, 2025 14:40
@rswanson rswanson force-pushed the swanny/i-need-more-validation branch from af1fc29 to 312ca45 Compare June 3, 2025 14:40
@rswanson rswanson merged commit bf5093d into main Jun 3, 2025
2 checks passed
Copy link
Member Author

rswanson commented Jun 3, 2025

Merge activity

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