-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Have local google_storage_bucket resource name validation #9086
Comments
I don't actually understand how it is acceptable to not validate. This wastes a lot of your users time, as the failures now occurs after PRs have been reviewed and merged, so then for every validation error I have to make a new PR, just to find out what is next. Is there some other solution that is being suggested to do validation? I think issuing warnings would be okay, or maybe calling actual API for validation. |
@rileykarson Dupe of #3580 and #8347 Futher details are in #8347 but plan time validation will never be perfect and end users are expected/required to be reading API documentation to supply valid values to Terraform. In short Terraform plan-time validation is broken/buggy as explained below:
Therefore, I advise you not to rely on plan time validation and leverage the API documentation for correct set of values. It will get easier with experience. |
That's all well and good, but I would point out that this proposed change is not asking for full plan-time validation. It's asking for a small amount of validation (more akin to sanity-checking actually) using a very simple regex on one of the seemingly more common pain points surrounding buckets.
Advising someone to not rely on plan-time validation is a bit like advising someone to not worry about what the unit tests say. If the unit tests don't work, you fix the unit tests. |
You are missing the point. For example, how would you even validate the examples I linked above? There isn't a way of validating those right now because of the limitations mentioned above. However, your proposal is valid but IMHO it won't be enough as there will always be resource #8437 is for fixing plan time validation and once there are satisfactory solutions we can potentially fix all of these validations problems. |
Name validation (for syntax, not uniqueness- you'd still fail at apply time if the name was non-unique) is a reasonable thing to add, and 3580/8437 are extremely general and don't really apply. For the provider team it's generally a matter of prioritization, see my reply at #9113 (comment). |
I do get the point. I completely understand that full validation isn't feasible right now (though like you, I absolutely wish it were). I accept that fact and concur that having the provider dev team spend all their time chasing down each API change and adding specific validation for them all would simply be an exercise in futility. My counterpoint, to continue to use the unit test analogy, was essentially "since full unit tests to catch everything aren't possible at the moment, let's at least get a sanity check in there to catch the most obvious things". If anything, reliance on plan-time validation (at least for this provider) should be referenced with a caveat emptor clause rather than the blanket "should not be used". |
b/237107072 |
Issue has been fixed now |
Resolved by GoogleCloudPlatform/magic-modules#6250 |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Community Note
Description
When creating buckets, there are some pretty straight forward naming guidelines. However, the provider doesn't seem to do any checks locally that would catch things like capital letters or special characters in the name, instead relying on the Google API to do so. I'd like to request that some of this validation be added so that it's caught in a
terraform plan
instead of interraform apply
.Per #3420, I totally understand that adhering strictly to the underlying API may not be feasible, but I think at least having a basic regex validator of something like
[a-z0-9][a-z0-9_.-]{1,220}[a-z0-9]
wouldn't be outrageous.New or Affected Resource(s)
Potential Terraform Configuration
References
The text was updated successfully, but these errors were encountered: