validation: Add ValidateRFC3339TimeString

[bflad]

Helper function so providers can easily ensure an attribute parses as time.RFC3339. We could also introduce a func TimeString(format string) schema.SchemaValidateFunc. Let me know if you think that would be better or if we should have both. 😄

[apparentlymart]

This seems like a fine idea to me! My feedback inline is specifically about the error messages that time.Parse produces, which seem to have the wrong target audience (Go developers, rather than Terraform users) and so we ought to do our best to "translate" the message to be more appropriate for our product.

Thinking about your TimeString alternative proposal, I think it'd be hard to achieve good error messages with a function that generic, so I think it's better to be more specific so that we can handle errors better.

I could see us having other functions that deal with different subsets of the format that RFC3339 allows (unqualified local time vs. requiring a timestamp, date-only references, etc) but it seems best to wait until we have those use-cases before we add them, and just focus on what you need right now.

[apparentlymart]

This error message confuses me... what does "Cannot parse ... as 2006" mean? Is this just Go exposing its weird time formatting representation?

I'd ideally like to try to do better than this and make the messages actually understandable to people who aren't Go experts, but I'm not sure how feasible that is based on what time.Parse returns. If this is the quality of the error messages from this function then I'd honestly rather just say "an RFC3339 timestamp is required" for all cases, since that at least offers some reasonable resolution (go and read RFC3339).

Perhaps we could improve on that generic message in some cases by some preprocessing:

• If it looks like the user is using wrong delimiters for either the date or the time, indicate that.
• If the timestamp is at the wrong granularity (only a date where a full time is required), indicate that.

The ParseError type suggests that we might be able to get some help with recognizing these situations from the time package itself, but the fields there are not documented so I'm not really sure what they mean and whether there's enough information here to recognize some common error cases. Maybe you can take a look at what's populated in ParseError for a few of these cases and see if it seems like something we could use to produce user-friendly messages for these scenarios?

[paultyng]

I think taking the layout in makes the most sense, otherwise you are going to have to potentially do this for all the constants? Considering the layout is a valid example time, I think the error messaging should be pretty straight forward as you can show an example for how to format it?

Alternatively you can format Now with the layout to get an example.

[apparentlymart]

Another thought on the generic error string idea: ideally we shouldn't be using a wide variety of different time formats across all of Terraform anyway, since that's not a great user experience. Perhaps we could standardize on always using RFC3339, regardless of what the underlying API uses, and convert where necessary to the underlying API?

I wouldn't do this in any case where the underlying representation has a different granularity than RFC3339, since that'd be confusing, but it seems right to me for full timestamps that can be unambiguously represented in that format.

[paultyng]

Oh I like this, having a standard TF time we translate to whatever the API wants...

But in that case maybe we should just call it TimeString but without a format?

[apparentlymart]

I like the explicitness of the current name but I don't feel that strongly about it, so I won't object if it's renamed.

[paultyng]

I just worry it implies keep adding others. But I don't feel strongly either! @bflad it's up to you!

…

On Fri, Mar 2, 2018, 6:05 PM Martin Atkins ***@***.***> wrote: I like the explicitness of the current name but I don't feel that strongly about it, so I won't object if it's renamed. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#17484 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZySgrFD78EuweYjtdcr_vTvvFSlj0Wks5tadAdgaJpZM4SZYkv> .

[bflad]

Thanks for the feedback, @paultyng and @apparentlymart! I've pushed this up so it now returns invalid RFC3339 timestamp instead of the cryptic time package errors. Can you please take another look?

Maybe in the future, we can look at the idea of a date/time being more of a first class type in Terraform schema to handle this logic a little more structurally: e.g. more descriptive error messaging, supporting short forms of the timestamp, standardizing on one format, etc. 😄

[ghost]

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.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.