Skip to content

Commit

Permalink
fix(events): update log event timestamp type be string or number
Browse files Browse the repository at this point in the history
This essentially reverts a change that was made in
#3576
(commit: b52e9b2).

The type has always been 'number' but we were in fact sending
strings to Garden Cloud which is the payload it expects.

Garden Cloud (version >= 1.360) now supports numbers but we need to
continue sending strings for backwards compatibility.

We can tighten the type to just 'number' once all Cloud instances are
up to date.
  • Loading branch information
eysi09 committed Jan 26, 2023
1 parent 0808531 commit 4241e45
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 4 deletions.
17 changes: 16 additions & 1 deletion core/src/events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,22 @@ export interface Events extends LoggerEvents {
}
watchingForChanges: {}
log: {
timestamp: number
/**
* Number of milliseconds since the epoch OR a date string.
*
* We need to allow both numberic and string types for backwards compatibility
* with Garden Cloud.
*
* Garden Cloud supports numeric date strings for log streaming as of v1.360.
* We can change this to just 'number' once all Cloud instances are up to date.
*
* Note that even though this has always been typed as a 'number' we've been
* attaching string timestamps to this payload because of missing types further
* up the pipe.
*
* TODO: Change to type 'number'.
*/
timestamp: number | string
actionUid: string
entity: {
moduleName: string
Expand Down
14 changes: 12 additions & 2 deletions core/src/plugin-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,18 @@ export type PluginEventLogContext = {
}

export type PluginEventLogMessage = PluginEventLogContext & {
/** number of milliseconds since the epoch */
timestamp: number
/**
* Number of milliseconds since the epoch OR a date string.
*
* We need to allow both numberic and string types for backwards compatibility
* with Garden Cloud.
*
* Garden Cloud supports numeric date strings for log streaming as of v1.360.
* We can change this to just 'number' once all Cloud instances are up to date.
*
* TODO: Change to type 'number'.
*/
timestamp: number | string

/** log message */
data: Buffer
Expand Down
11 changes: 10 additions & 1 deletion core/src/plugins/kubernetes/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,16 @@ export class PodRunner extends PodRunnerParams {
void stream.forEach((entry) => {
const { msg, timestamp } = entry
events.emit("log", {
timestamp: timestamp?.getTime() || new Date().getTime(),
// This should be a numeric value (i.e. timestamp.getTime()) as opposed to a
// date string to be consistent with other event timestamps.
//
// However, due to missing types this has been a string value without us really noticing and
// and that's the shape Cloud expects. This was (rightly) changed to a numeric value with
// https://github.com/garden-io/garden/pull/3576 (b52e9b298f7c59b8210a77edc4c451301daa76e3)
// but we need to revert for Cloud backwards compatibility.
//
// TODO: Change to type number when all Cloud instance versions are >= v.1360.
timestamp: timestamp?.toISOString() || new Date().toISOString(),
data: Buffer.from(msg),
...logEventContext,
})
Expand Down

0 comments on commit 4241e45

Please sign in to comment.