feat: Add Time configtype

[rhino1998]

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.

kind: source
spec:
  name: "orb"
  registry: "grpc"
  path: "localhost:7777"
  version: "v1.0.0"
  tables:
    ["*"]
  destinations:
    - "sqlite"
  spec:
    api_key: "${CQ_ORB_API_KEY}"
    timeframe_start: 10 days ago
    timeframe_end: "2024-09-25T03:52:40.14157935-04:00"

Here timeframe_start and timeframe_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 having

timeframe_start: 10 days ago
timeframe_end: now

or even omitting timeframe_end entirely is more useful

timeframe_start: 10 days ago

The plugin is free to define default like so

type Spec struct {
	// TimeframeStart is the beginning of the timeframe in which to fetch cost data.
	TimeframeStart configtype.Time `json:"timeframe_start"`

	// TimeframeEnd is the beginning of the timeframe in which to fetch cost data.
	TimeframeEnd configtype.Time `json:"timeframe_end"`

	// ...
}


func (s *Spec) SetDefaults() {
	if s.TimeframeStart.IsZero() {
		s.TimeframeStart = configtype.NewRelativeTime(-1 * 365 * 24 * time.Hour)
	}

	if s.TimeframeEnd.IsZero() {
		s.TimeframeEnd = configtype.NewRelativeTime(0)
	}
}

[rhino1998]

Added some more context. Should configtype.Time.Time take a time.Time instead of a func() time.Time?

[hermanschaaf]

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 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.

[erezrokah]

the library was last updated 4 years ago

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

well-defined number of formats.

Agree it does more than we need, but timeframe_start: -1000h seems more machine friendly than human friendly to me

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.

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?

[hermanschaaf]

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 2024-05-01T00:00:00Z, or a default value, often relative (30 days ago or now). This doesn't lend us the flexibility we desire for scheduled syncs in Cloud

[hermanschaaf]

Agree it does more than we need, but timeframe_start: -1000h seems more machine friendly than human friendly to me

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:

• now
• x seconds [ago|from now]
• x minutes [ago|from now]
• x hours [ago|from now]
• x days [ago|from now]

would be a good enough start (we can always add more if requested)

[erezrokah]

I would maybe say something like:

• now
• x seconds [ago|from now]
• x minutes [ago|from now]
• x hours [ago|from now]
• x days [ago|from now]

would be a good enough start (we can always add more if requested)

Sounds like a plan to me

[hermanschaaf]

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. 2024-05-10, for plugins that need it.

[hermanschaaf]

@erezrokah @rhino1998 I lost track a bit, is there still anything unresolved on this PR?

[erezrokah]

@erezrokah @rhino1998 I lost track a bit, is there still anything unresolved on this PR?

Not sure https://github.com/cloudquery/plugin-sdk/pull/1905/files#r1774764467 is resolved, also https://github.com/cloudquery/plugin-sdk/pull/1905#discussion_r1775148934

I'd rather if we don't change the existing type behavior as that implicitly change spec parsing behavior for plugins. If plugins want to support the new human friend format mixed duration/time format they should do it explicitly I think

[rhino1998]

@erezrokah @rhino1998 I lost track a bit, is there still anything unresolved on this PR?

Not sure https://github.com/cloudquery/plugin-sdk/pull/1905/files#r1774764467 is resolved, also #1905 (comment)

I'd rather if we don't change the existing type behavior as that implicitly change spec parsing behavior for plugins. If plugins want to support the new human friend format mixed duration/time format they should do it explicitly I think

The new mixed duration/time format is opt-in, but the new duration format does extend the old plain time.ParseDuration-based format.

How would you prefer the opt-in be done? A DurationV2 type? A separate package?

[erezrokah]

How would you prefer the opt-in be done? A DurationV2 type? A separate package?

I don't mind I mostly don't want to change the existing behavior of the current type. You can also put everything under the new Time type without a separate type

[erezrokah]

Nothing blocking on my end, so you can merge at will. We might want to wait with releasing the new SDK until after today's plugins release cycle to avoid creating noise for the plugins teams

[disq]

I think (sorry for the late comment) the from now syntax can be confusing, as people could expect 2 days from <some date here> or worse, 2 days from new year's eve to work.

[rhino1998]

I think (sorry for the late comment) the from now syntax can be confusing, as people could expect 2 days from <some date here> or worse, 2 days from new year's eve to work.

We could pretty easily add support for stuff like 2 days from <timestamp>. That actually generalizes the input pretty nicely and will simplify/merge validation cases

[rhino1998]

@disq Do you consider the above blocking?

[disq]

@disq Do you consider the above blocking?

@rhino1998 No not at all.

[erezrokah]

Not sure what's the status here, should we merge this or close it as stale?