Converted back to draft as I work on adding tests for the FormatHealthStatusResponse function.

@derailed I pushed the changes you've requested. I will rebase once you're OK with the changes.

/test

@darox seems like some tests are failing, would you mind rebasing PR, hopefully it fixes it 😃 ?

@darox seems like some tests are failing, would you mind rebasing PR, hopefully it fixes it 😃 ?

I need a bit more time, because there's now a conflict:

+++ b/pkg/health/client/client.go
@@@ -325,29 -315,8 +325,34 @@@ func formatNodeStatus(w io.Writer, nod
        if localhost {
                localStr = " (localhost)"
        }
++<<<<<<< HEAD
 +      if succinct {
 +              if printAll || !nodeIsHealthy(node) {
 +                      ips := []string{getPrimaryAddressIP(node)}
 +                      for _, addr := range GetHostSecondaryAddresses(node) {
 +                              if addr == nil {
 +                                      continue
 +                              }
 +                              ips = append(ips, addr.IP)
 +                      }
 +                      hostStatuses := SummarizePathConnectivityStatusType(GetAllHostAddresses(node))
 +                      endpointStatuses := SummarizePathConnectivityStatusType(GetAllEndpointAddresses(node))
 +
 +                      fmt.Fprintf(w, "  %s%s\t%s\t%d/%d", node.Name, localStr, strings.Join(ips, ","), hostStatuses[ConnStatusReachable], len(GetAllHostAddresses(node)))
 +                      if hostStatuses[ConnStatusUnknown] > 0 {
 +                              fmt.Fprintf(w, " (%d unknown)", hostStatuses[ConnStatusUnknown])
 +                      }
 +                      fmt.Fprintf(w, "\t%d/%d", endpointStatuses[ConnStatusReachable], len(GetAllEndpointAddresses(node)))
 +                      if endpointStatuses[ConnStatusUnknown] > 0 {
 +                              fmt.Fprintf(w, " (%d unknown)", endpointStatuses[ConnStatusUnknown])
 +                      }
 +                      fmt.Fprintf(w, "\n")
 +              }
 +      } else {
++=======
+

@marseel I rebased and fixed the conflicts, let's see if it goes through.

I don't see how this PR would affect the failing CI task.

I've rerun test, let's see if it was just a flake (probably)

I've rerun test, let's see if it was just a flake (probably)

passed

@ldelossa can we get it merged?

/test

/test

@ldelossa would you mind taking a look or should we reassign?

Backport to v1.16 in #35468 lead to non-trivial merge conflicts, this will need a manual backport. Marking as backport/author.

What are the steps I have to take, to get this backported to v1.16?

What are the steps I have to take, to get this backported to v1.16?

Hey @darox 👋 Are you still willing to take care of this author backport?

If so, the easiest approach is probably to:

1. Check out the v1.16 branch
2. Execute ./contrib/backporting/cherry-pick bf7bc43bd53ca77934513ffd887a59974deca24c
3. Fix the conflicts
4. Run git am --continue
5. Amend the commit to add a description of the addressed conflicts (e.g., ce33255)
6. Open a PR against the v1.16 branch.
7. Once merged, change the label of this one to backport-done/v1.16

Feel free to reach out to me if you have questions.

@giorio94 If I will find time, I will take a look.

I had some time to check the code and I think we first need to apply fee6107 before I can apply my commit. My whole commit is based on the former commit. Now, the question is, if we can apply fee6107 because it deprecates some metrics. This commit was first applied on 1.17. I pushed it to #37463 which fixes it. But someone will have to decide, whether that's fine.

Now, the question is, if we can apply fee6107 because it deprecates some metrics.

That commit introduces a new feature, which is typically not eligible for being backported, and IMO it would come with a significant risk for stable releases. If that commit is a strict requirement to backport this PR, my feeling is that it would be better skipping this backport, considering that it fixes a CLI command that most likely always suffered from this bug.

Now, the question is, if we can apply fee6107 because it deprecates some metrics.

That commit introduces a new feature, which is typically not eligible for being backported, and IMO it would come with a significant risk for stable releases. If that commit is a strict requirement to backport this PR, my feeling is that it would be better skipping this backport, considering that it fixes a CLI command that most likely always suffered from this bug.

Yes, I would agree with that.

Ack, labels dropped.