Fix: Freight should manage its own discoveryTimestamp, not rely on cr…#6133
Fix: Freight should manage its own discoveryTimestamp, not rely on cr…#61332003Aditya wants to merge 1 commit intoakuity:mainfrom
Conversation
…eationTimestamp akuity#5855 Signed-off-by: Aditya Mishra <mishraaditya675@gmail.com>
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // DiscoveryTimestamp is the time at which the Freight was first discovered | ||
| // from its source. This is distinct from metadata.creationTimestamp, which is | ||
| // set by Kubernetes and cannot be controlled by users. | ||
| DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,11,opt,name=discoveryTimestamp"` |
There was a problem hiding this comment.
It appears codegen wasn't run after this change. Among many other things, codegen will also set the protobuf struct tag for you (making it harder to get wrong).
I would suggest this change:
| DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,11,opt,name=discoveryTimestamp"` | |
| DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty"` |
When you run codegen (make hack-codegen), which you will need to do for reasons other than just this, it will probably put it back to exactly as you had it here, but it may do something slightly different. Better safe than sorry.
| // DiscoveryTimestamp is the time at which the Promotion was created. | ||
| // This is distinct from metadata.creationTimestamp, which is | ||
| // set by Kubernetes and cannot be controlled by users. | ||
| DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,4,opt,name=discoveryTimestamp"` |
There was a problem hiding this comment.
Same comment as here: https://github.com/akuity/kargo/pull/6133/changes#r3118215938
There was a problem hiding this comment.
Can we call this startedAt? That's the field name we use in stepExecutionMetadata, and "discovery" is more of a freight thing.
apiVersion: kargo.akuity.io/v1alpha1
kind: Promotion
metadata:
creationTimestamp: 2026-04-08T15:50:21Z
name: dev.01knpwk5z8ngczawgqctc79w39.efe1b47
status:
startedAt: 2026-04-08T15:40:22Z # <<<<< my preference
finishedAt: 2026-04-08T15:51:10Z
phase: Succeeded
stepExecutionMetadata:
- alias: task-1::step-1
finishedAt: 2026-04-08T15:50:24Z
startedAt: 2026-04-08T15:50:22Z
status: SucceededThere was a problem hiding this comment.
Also, unlike Freight, for the promotion object, this needs to be under status. Not a top-level field.
| lhsTime := lhs.DiscoveryTimestamp | ||
| rhsTime := rhs.DiscoveryTimestamp | ||
| if lhsTime == nil || rhsTime == nil { | ||
| return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time) | ||
| } | ||
| return rhsTime.Time.Compare(lhsTime.Time) |
There was a problem hiding this comment.
This was really smart to fall back on CreationTimestamp since existing Freight will obviously not already have a DiscoveryTimestamp.
I might suggest a small improvement though. Instead of falling back on CreationTimestamp for both when either doesn't have a DiscoveryTimestamp, I would let each of them fall back individually like this:
| lhsTime := lhs.DiscoveryTimestamp | |
| rhsTime := rhs.DiscoveryTimestamp | |
| if lhsTime == nil || rhsTime == nil { | |
| return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time) | |
| } | |
| return rhsTime.Time.Compare(lhsTime.Time) | |
| lhsTime := lhs.DiscoveryTimestamp | |
| if lhsTime == nil { | |
| lhsTime = lhs.CreationTimestamp.Time | |
| } | |
| rhsTime := rhs.DiscoveryTimestamp | |
| if rhsTime == nil { | |
| rhsTime = rhs.CreationTimestamp.Time | |
| } | |
| return rhsTime.Time.Compare(lhsTime.Time) |
| // Sort the terminal Promotions by discovery time in descending order | ||
| slices.SortFunc(promotions.Items, func(lhs, rhs kargoapi.Promotion) int { | ||
| lhsTime := lhs.DiscoveryTimestamp | ||
| rhsTime := rhs.DiscoveryTimestamp | ||
| if lhsTime == nil || rhsTime == nil { | ||
| return rhs.CreationTimestamp.Compare(lhs.CreationTimestamp.Time) | ||
| }) | ||
| } | ||
| return rhsTime.Time.Compare(lhsTime.Time) |
There was a problem hiding this comment.
Same comment as here: https://github.com/akuity/kargo/pull/6133/changes#r3118278060
I start to wonder whether we should include a generic SortByDiscoveryDate() helper somewhere.
| lhsTime := lhs.DiscoveryTimestamp | ||
| rhsTime := rhs.DiscoveryTimestamp | ||
| if lhsTime == nil || rhsTime == nil { | ||
| return lhs.CreationTimestamp.Time.Compare(rhs.CreationTimestamp.Time) | ||
| } | ||
| return rhsTime.Time.Compare(lhsTime.Time) |
There was a problem hiding this comment.
Same as here: https://github.com/akuity/kargo/pull/6133/changes#r3118302803
Another point in favor of a generic helper.
|
This looks pretty good overall @2003Aditya. Thank you for doing this. One spot(s) I think may have been overlooked for sorting would be any API endpoints that return a list of Promotions. |
| // DiscoveryTimestamp is the time at which the Promotion was created. | ||
| // This is distinct from metadata.creationTimestamp, which is | ||
| // set by Kubernetes and cannot be controlled by users. | ||
| DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,4,opt,name=discoveryTimestamp"` |
There was a problem hiding this comment.
Can we call this startedAt? That's the field name we use in stepExecutionMetadata, and "discovery" is more of a freight thing.
apiVersion: kargo.akuity.io/v1alpha1
kind: Promotion
metadata:
creationTimestamp: 2026-04-08T15:50:21Z
name: dev.01knpwk5z8ngczawgqctc79w39.efe1b47
status:
startedAt: 2026-04-08T15:40:22Z # <<<<< my preference
finishedAt: 2026-04-08T15:51:10Z
phase: Succeeded
stepExecutionMetadata:
- alias: task-1::step-1
finishedAt: 2026-04-08T15:50:24Z
startedAt: 2026-04-08T15:50:22Z
status: Succeeded| // DiscoveryTimestamp is the time at which the Promotion was created. | ||
| // This is distinct from metadata.creationTimestamp, which is | ||
| // set by Kubernetes and cannot be controlled by users. | ||
| DiscoveryTimestamp *metav1.Time `json:"discoveryTimestamp,omitempty" protobuf:"bytes,4,opt,name=discoveryTimestamp"` |
There was a problem hiding this comment.
Also, unlike Freight, for the promotion object, this needs to be under status. Not a top-level field.
|
@jessesuen @krancour Okay, I will change |
| Namespace: stage.Namespace, | ||
| Annotations: annotations, | ||
| }, | ||
| DiscoveryTimestamp: &metav1.Time{Time: time.Now()}, |
There was a problem hiding this comment.
Since I'm asking for StartedAt timestamp to be a status thing, this will no longer be appropriate to set here. It's also something that the controller would need to set when it transitions the Promotion from Pending to Running.
3 participants
#5855