search

kubernetes-sigs / cli-utils

This repo contains binaries that built from libraries in cli-runtime.
Apache License 2.0
155 stars 78 forks source link

feat: improve event status consistency #570

Closed karlkfi closed 2 years ago

karlkfi commented 2 years ago

Event Changes

  1. Rename Event "Operation" to "Status" to better reflect the meaning of the current enum values.
  2. Ensure all Apply/Prune/Delete/Wait* event status support:
    1. Pending
    2. Successful
    3. Skipped
    4. Failed
  3. Combine Applied, Changed, Unchanged, and ServersideApplied into ApplySuccessful
    1. Use ApplySkipped instead of Unchanged when an object is skipped/filtered.
    2. Use ApplySuccessful instead of Unchanged when client-side-apply skips the PATCH because the GET detects no changes.
  4. Rename ActionGroupEvent "Type" to "Status" for consistency** with the other event status.

* Timeout is only currently possible for Wait events, but we may add it to other events status in the future. For now, those events and API calls do not have a client-side timeout.

** Action groups will continue to use "Started" & "Finished" as their status enum values, because action groups have no success or failure mode, so renaming them Pending and Successful would be misleading, but if we ever decide to add an error, then renaming them might make sense for consistency.

Printer Changes

All Printers

  1. Print new short Status enum string wherever Operation was printed before
    • Pending
    • Successful
    • Skipped
    • Failed
    • Timeout
  2. Track failure stats by checking the status field is -Failed, not just err != nil (Skip events may also error as of https://github.com/kubernetes-sigs/cli-utils/pull/562).

Event Printer

  1. Add a summary at the end of the Event printer to report stats.
    • This replaces the current mechanism that prints stats after the last ActionGroup of each type, because that method is hard to understand (apply & prune wait stats are combined).
  2. Replace past tense short hand with more consistent two word status:
    • applied -> apply successful
    • pruned -> prune successful
    • deleted -> delete successful
  3. Add output lines for action group started & finished events:
    • Format: "<action> phase <status>" (e.g. "apply phase started")
    • This replaces the stats that were printing on the last finished of each group type.
    • This should help explain why apply/prune/delete/wait aren't all done at the same time, and make it more obvious that the actuation is phased/grouped.
    • This includes inventory action group events, which previously were not printed, making it more transparent what is happening and easier to debug inventory update failures.

JSON Printer

  1. Rename the "operation" field to "status" to match the event change.
  2. Remove the "eventType" field, because it duplicates the "type" + "status" fields.
  3. Add a "summary" line to print the aggregated summary stats after all other events have been consumed.
    • One summary line for each action group type:
      • Apply
      • Prune
      • Delete
      • Wait
    • Format similar to existing stats fields:
      • action: "Apply|Prune|Delete|Wait"
      • count: %d
      • successful: %d
      • skipped: %d
      • failed: %d
      • timeout: %d (wait summary only)
      • timestamp: %s (ISO 8601)
      • type: "summary"

Table Printer

  1. Print "Pending" instead of the empty string in the status column, once a pending event has been received.
    • Previously pending events were ignored
    • This should make it clearer in table view which objects are part of the current action group (aka phase) without having to add a new column to say so explicitly.

BREAKING CHANGES

karlkfi commented 2 years ago

I expect example tests to fail here, because I haven't updated them with the new event printer changes yet.

Liujingfang1 commented 2 years ago

Overall the change looks good to me.

+1 one on the new printer output that includes the total number.

expectedOutputLine "reconcile result: 2 attempted, 2 successful, 0 skipped, 0 failed, 0 timed out"
k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karlkfi, mortent

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/cli-utils/blob/master/OWNERS)~~ [mortent] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
karlkfi commented 2 years ago

Rebased. Tests passed. Needs another LGTM.

mortent commented 2 years ago

/lgtm