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 validation for start_time to resource_policy #2940

Merged
merged 3 commits into from
Jan 9, 2020

Conversation

chrisst
Copy link
Contributor

@chrisst chrisst commented Jan 8, 2020

Fixes hashicorp/terraform-provider-google#4611

Release Note Template for Downstream PRs (will be copied)

`compute`: Added validation for `compute_resource_policy` to no longer allow invalid `start_time` values that weren't hourly.

@chrisst chrisst requested a review from emilymye January 8, 2020 19:45
@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, e672143.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#1603
depends: GoogleCloudPlatform/terraform-google-conversion#311
depends: hashicorp/terraform-provider-google#5342

Copy link
Contributor

@emilymye emilymye left a comment

Choose a reason for hiding this comment

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

Can we also add a note to api.yaml? to make it more explicit they dont have a choice for MM (right now it says HH:MM where MM is 00, so we might as well say an hourly time of HH:00 instead)

func validateHourlyOnly(val interface{}, key string) (warns []string, errs []error) {
v := val.(string)
parts := strings.Split(v, ":")
if len(parts) != 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to ignore this but we also might as well verify the hour is between 0 and 23 at this point? Part of me thinks you should make this a regexp match or do time.Parse("15:04", v) (even though I hate how they make you define a custom format)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't have to validate hours because the api will reject bad inputs there whereas the minutes it would accept and silently transform. But it's probably worth throwing it in anyway.

re: time.parse or regex - it's going to be a significantly more complex operation to use either of those over string splitting and casting to ints and I don't think either strategy provides added value in readability or flexibility.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, c9d8ee3.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

chrisst and others added 3 commits January 9, 2020 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google_compute_resource_policy easy to misuse
4 participants