-
Notifications
You must be signed in to change notification settings - Fork 330
Feature: CLI list deployments shows health for each entry #1594
Conversation
internal/core/app_status_report.go
Outdated
@@ -345,6 +344,9 @@ func (op *statusReportOperation) Do( | |||
return nil, status.Errorf(codes.FailedPrecondition, "unsupported status operation type") | |||
} | |||
|
|||
// Add the time generated to the outer status report | |||
realMsg.TimeGenerated = report.TimeGenerated |
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.
This feels like something the UI will likely want to include (cc @jgwhite ), so I added it to the top level StatusReport. It also means we don't need to dig into the lower-level status report in the CLI at all anymore.
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.
Yes, this is going to be super helpful in the UI too.
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.
for _, statusReport := range statusReportsResp.StatusReports { | ||
if deploymentTargetId, ok := statusReport.TargetId.(*pb.StatusReport_DeploymentId); ok { | ||
if deploymentTargetId.DeploymentId == b.Id { | ||
switch statusReport.Health.HealthStatus { |
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.
Happy to see we’re switching on the outer HealthStatus
rather than the SDK’s Health
enum.
fbc6cd3
to
c672e93
Compare
c672e93
to
2999ee0
Compare
2999ee0
to
34e69f2
Compare
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.
Excited to have this! I had a couple questions and one nit.
internal/core/app_status_report.go
Outdated
@@ -345,6 +344,9 @@ func (op *statusReportOperation) Do( | |||
return nil, status.Errorf(codes.FailedPrecondition, "unsupported status operation type") | |||
} | |||
|
|||
// Add the time generated to the outer status report | |||
realMsg.GeneratedTime = report.TimeGenerated |
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.
:chefs-kiss:
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.
Why is one AB
and the other BA
(A = "Generated" B = "Time")?
Renaming fields is backwards compatible in proto so should we rename fields?
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.
Good catch...yeah these should be named the same.
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.
Ha, I actually just changed this after @jgwhite caught that many similar time fields that the server returns are in the AB order, and it might be better to be consistent with other server fields than with the sdk field that we're mirroring: #1594 (comment)
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.
If renaming fields is backwards compatible, fixing the SDK field may be the way to go.
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.
realMsg.GeneratedTime
was added in this PR so we should match the naming from the SDK.
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.
Ah, I noticed Jamies comment. We could flip them in the SDK if it doesn't break backwards compatibility. It's too bad this wasn't caught sooner 😄
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.
Sweeeet, thanks for the fix izaak 😄 A couple of minor comments from me and some +1s on the other feedback given 👍🏻
4c3d1b4
to
2bdb354
Compare
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.
@izaaklauer I think we should fix the GeneratedTime = TimeGenerated
issue here. We should either flip it back to TimeGenerated
on the copied convenience server proto var to match the option in the raw report (and be a little inconsistent), or try to fix the name on the SDK side if it doesn't break backwards compatibility so it is consistent with other parts of the API. If we do change it, it'd be good to get it in a 0.4.1 before anyone really builds anything on top of this to reduce major breaking changes.
0a05710
to
aa9b2cb
Compare
Matches our convention of other time values. Prerequisite for hashicorp/waypoint#1594 (discussion hashicorp/waypoint#1594 (review))
PR to rename the time field in waypoint-plugin-sdk: hashicorp/waypoint-plugin-sdk#35. I tested using that change in a new server with an old version of the CLI and vice-versa, and the backwards compatibility works! Once the sdk change is merged, I'll update this pr to include it. |
Nice @izaaklauer ! You'll probably also need to update all of the plugins to use the new variable too when they finish generating a report, right? Maybe that can be done in a new PR when we update the SDK version this project is using. |
In writing this, I realize that our ListStatusReports endpoing lists _all_ status reports for all resources, with no way to filter. We'll probably need to enhance that pretty soon as we build up more report types and volume. Also it looks like this is a big protobuf diff. The existing generated code has "unknown" protobuf version, so i'm not sure what version I need to revert to to make it match.
Co-authored-by: Jamie White <[email protected]>
Regenerated protobufs
Previously, if a plugin does not return status reports, we would say "Unknown Status", which is a bit wordy and also conflicts with the "UNKNOWN" official status, where a plugin _has tried_ to get status and is explicitly returning unknown. "n/a" kind of implies that the very concept of health doesn't apply to some deployments, which isn't right either, but it's the least-worst thing I can think of.
Also looping in Rae's fix (https://github.com/hashicorp/waypoint/pull/1596/files)
aa9b2cb
to
ddb18b4
Compare
The other plugin changes weren't too big and addresses this feedback, so I looped it in here. We are now consistently using |
3064757
to
565d7ae
Compare
Note that because with this change we're recording and reading GeneratedTime from the outer protobuf, and reading HealthStatus from the outer protobuf. We used to read these from the inner sdk pb.any protobuf. This means that (deployment 3 was made with 0.4.0, and 4 with this new version. Note that 3 is missing health info). The UI is also unaffected, because it doesn't inspect the inner sdk proto. With the current implementation of health checks, health is only determined immediately after a deployment, and is most valuable at that point, so losing that historical health check data in this CLI view may be acceptable. The next deployment that anyone runs after upgrading waypoint will show health on I would argue that it's not worth the complexity to check the SDK proto in the case where the outer proto is blank, but I'm not wedded to that opinion. |
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.
⏳
Previously, running
waypoint deployment list
would attach the most recent health check of any kind to the most recent deployment. With this change, we display the health of each deployment listed.Example:
In writing this, I realize that our ListStatusReports endpoint lists all status reports for all resources, with no way to filter to a specific resource or recency limit. We'll probably need to enhance that pretty soon as we build up more report types and volume.
Also it's a big protobuf diff, but it should get us back to the the code produced by the version of protoc locked in by go.mod.