-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: Add Time configtype #1905
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.
Hi @rhino1998, nice 🚀 Can you give an example how this will be used in a plugin and in a spec file?
Added some more context. Should |
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 like the idea of having this kind of type as an SDK helper.
A few comments:
- If the purpose is to make it easier to configure relative times, I would use https://github.com/tj/go-naturaldate/tree/master instead of a wrapper over duration
- I would hide the
IsRelative
andNewRelativeTime
and doNewTime(t string)
that accepts either an RFC3339 or a natural date string. I don't see a reason consumers need to know the internal representation, also I don't think we even needtyp timeType
, as you can haveNewTime(t string, base time.Time)
and convert the string on creation.
configtype/time.go
Outdated
|
||
var err error | ||
switch { | ||
case timeRFC3339Regexp.MatchString(s): |
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.
Instead of using regular expressions can't we try to parse with each format, then return an error if all formats failed to parse?
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.
Yeah, although we still need regexes for the jsonschema definitions
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.
Also, the regexes let us give a more specific error, otherwise we'd always error with a duration parsing error (or whatever the last parsing case is).
I would push back on this; the library was last updated 4 years ago, and supports much more than we need to support. I think we would be better off supporting a small, well-defined number of formats. Also I think the purpose here is not to make it easier, it's to make it possible at all (in Cloud), in a standardized way. |
I agree it hasn't change in a long time, but it doesn't have any other dependencies (other than the ones used for testing), and the author is https://github.com/tj (https://x.com/tjholowaychuk), which has a bunch of other popular repositories like https://github.com/tj/commander.js and https://github.com/koajs/koa
Agree it does more than we need, but
Can you add more context on the Cloud use case? We want the backend to specify a duration via spec generated via backend code in Go? If so is it possible to convert the duration to time before? |
We want users to be able to specify a relative time, such as "30 days ago", which will be stored as part of the spec. Every time the sync runs, this should be converted to a timestamp relative to the current time. This way, we don't need to involve the backend to do string interpolation on the spec, and things remain relatively simple. Right now, what happens in many plugins is they allow only a fixed timestamp to be specified, such as |
I agree with this. I'm also okay with using that library, but if we do, let's not support everything that the library supports, but instead limit it to a defined list of supported formats that we can control. I would maybe say something like:
would be a good enough start (we can always add more if requested) |
Sounds like a plan to me |
I forgot that in addition to relative times, we'd also want to support fixed timestamps of course, probably using RFC 3339. And also some support for plain dates, e.g. |
Summary
This adds a new Time type to be used in plugin specs that can represent either fixed times or relative times. A future PR should extend the duration parsing to handle days, etc.
Here
timeframe_start
andtimeframe_end
are both configtype.Time types. Using mixed relative/fixed timestamps for timeframes isn't really useful here as the relative one will be relative to the run-time of the sync but in this example havingor even omitting
timeframe_end
entirely is more usefulThe plugin is free to define default like so