-
-
Notifications
You must be signed in to change notification settings - Fork 89
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 support for date/time fields to firewall_rule #173
base: main
Are you sure you want to change the base?
Conversation
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 this! I think implementation looks good, mostly some comments about format and consistency with other providers. Things that are lists can possibly be made sets so they are easy to build programmatically if necessary, and date/time handling is typically done in RFC 3339.
Open to your thoughts on usage here though, if it makes more sense to pass through from the controller verbatim for some other reason?
@@ -169,6 +169,51 @@ func resourceFirewallRule() *schema.Resource { | |||
Optional: true, | |||
ValidateFunc: validation.StringInSlice([]string{"match-ipsec", "match-none"}, false), | |||
}, | |||
"weekdays": { | |||
Description: "Days of the week the rule is enforced.", | |||
Type: schema.TypeString, |
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.
This should probably be a set, similar to src_firewall_group_ids
:
Type: schema.TypeString, | |
Type: schema.TypeSet, | |
Elem: &schema.Schema{Type: schema.TypeString}, |
}, | ||
"monthdays": { | ||
Description: "Days of the month the rule is enforced.", | ||
Type: schema.TypeString, |
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.
Set like weekdays
.
"startdate": { | ||
Description: "Date that the rule starts being enforced.", | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"starttime": { | ||
Description: "Time that the rule starts being enforced.", | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"stopdate": { | ||
Description: "Date that the rule stops being enforced.", | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, | ||
"stoptime": { | ||
Description: "Time that the rule stops being enforced.", | ||
Type: schema.TypeString, | ||
Optional: true, | ||
}, |
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.
Can you use a date without a time? Or can you only use them together?
Typically for date/time we use RFC 3339 format, since its a bit easier for international usage. I think if they have to be used together, I'd prefer a single start
attribute that did both date and time in RFC 3339 format. We have some validation stuff in the SDK for this: https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/helper/validation#IsRFC3339Time
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.
If they are used separately (which I believe is the case), we should still probably use date/time formats that are closer to RFC 3339 as that is how Terraform's built-in functions work, for example: https://www.terraform.io/docs/language/functions/timeadd.html
I'm unable to find these in the UI, do you know what controller version would have these? |
Initial support for date/time firewall rules.