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.
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
HCCP91 hvn routes data source #115
HCCP91 hvn routes data source #115
Changes from 11 commits
1ec05d0
fb37e9b
669860d
5472d41
3523aff
66f70f7
a3c5712
fa10e8e
7a41938
e8b260b
cac5286
ebe9f22
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added validation for
destination_cidr
. Originally I was planning to use the IsCIDR helper as @bcmdarroch had suggested, and usevalidation.All
following the example here(https://github.com/hashicorp/terraform-provider-aws/blob/main/aws/resource_aws_eks_cluster.go#L133). However, I went with a custom validator for 3 reasons:1- terraform-provider-hcp only uses
ValidateDiagFunc
, andvalidation.All
(https://github.com/hashicorp/terraform-plugin-sdk/blob/v1.17.2/helper/validation/meta.go#L30) would not work2-
ValidateFunc
is deprecated (https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/schema.go#L308) but most importantly3- HCP create HVN route endpoint validation (https://github.com/hashicorp/cloud-network/blob/master/service/network/private/service_create_hvn_route.go#L218) uses the validation rule here: https://github.com/hashicorp/cloud-sdk/blob/master/validation/rule_cidrblock.go#L0-L1; this validation is reused in several HCP APIs taking a CIDR parameter.
validateIsStartOfPrivateCIDRRange
validates based on the same rules as the cloud-sdk CIDR validation:a) CIDR is valid
b) CIDR is private CIDR block
c) Address is at beginning of CIDR range
We could reuse this validator elsewhere in the provider too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to remove validation here (and probably in other places too), the least duplicated logic we have in Tf provider the easier it will be to maintain. Basic request validation will be done by the backend very quickly and won't impact UX of the TF provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding client-side validation for this! Looks great, and agreed that this validator should be used for our other CIDRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, just missed @smaant 's response on this. IMO it is reasonable to have this be validated client-side (I'm assuming it is generic and unlikely to change over time), and another slight benefit is that the user will get feedback on the
plan
step instead of seeing it later duringapply
. But otherwise, I do agree that this could be deferred to the backend and the experience would be similar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced the custom validator with
IsCIDR
as we are looking to gain some traction on this work, and merged this PRAlso spoke to provider team (@bcmdarroch & @roaks3 ) as well as some Terraform practitioners; my recommendation is to ensure the custom validator is added back at some point. The aforementioned 3 rules are rudimentary and are unlikely to be removed from the cloud-sdk validation package, hence drift is less of a concern. If the validation rules in cloud-sdk were to change, we are unlikely to remove rules, and only likely to add additional rules, in which case the custom validator covering 3 existing rules will be a good idea in terms of improving the practitioner experience for this provider.