Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validators to transport node resource #1111

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

annakhm
Copy link
Collaborator

@annakhm annakhm commented Feb 12, 2024

When user provides empty strings or empty lists, NSX fails the request without specifying what exactly is missing. It is preferrable to fail those invalid configs on plan stage.

When user provides empty strings or empty lists, NSX fails the
request without specifying what exactly is missing. It is preferrable
to fail those invalid configs on plan stage.

Signed-off-by: Anna Khmelnitsky <[email protected]>
@annakhm annakhm force-pushed the add-transport-node-validation branch from 52c7b50 to 247d896 Compare February 12, 2024 22:39
@annakhm
Copy link
Collaborator Author

annakhm commented Feb 13, 2024

/test-all

@GraysonWu
Copy link
Collaborator

When user provides empty strings or empty lists, NSX fails the request without specifying what exactly is missing. It is preferrable to fail those invalid configs on plan stage.

I'm wondering if the same issue also exists in other resources.

Copy link
Collaborator

@ksamoray ksamoray left a comment

Choose a reason for hiding this comment

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

LGTM

nsxt/resource_nsxt_edge_transport_node.go Show resolved Hide resolved
Copy link
Member

@salv-orlando salv-orlando left a comment

Choose a reason for hiding this comment

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

LGTM

@salv-orlando
Copy link
Member

When user provides empty strings or empty lists, NSX fails the request without specifying what exactly is missing. It is preferrable to fail those invalid configs on plan stage.

I'm wondering if the same issue also exists in other resources.

While I agree it might be best to fail in plan stage and I also think the issue will exist in other resources, we should also not forget that we don't want to do validation in place of NSX API, as this will create logic duplication and possibly create a bit of complexity in Interop.

It's fine though to pre-validate those field which should not be empty, as that's not something that should be expected to change in NSX API evolution. Regardless I would not scan all existing resources for these case, maybe let's just consider doing this in auto-generated code.

@annakhm
Copy link
Collaborator Author

annakhm commented Feb 13, 2024

When user provides empty strings or empty lists, NSX fails the request without specifying what exactly is missing. It is preferrable to fail those invalid configs on plan stage.

I'm wondering if the same issue also exists in other resources.

I'm reluctant to apply this validator to released resources, since this can break backwards compatibility. But we can go over resources in Beta and improve validation there.

@annakhm annakhm merged commit a7ff20d into master Feb 13, 2024
6 of 7 checks passed
@annakhm annakhm deleted the add-transport-node-validation branch August 2, 2024 15:19
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.

4 participants