Add wrapper func for spew.Sprintf in utils

[haoruan]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
A wrapper of spew.Sprintf, so to pretty print object with a util function

Which issue(s) this PR fixes:

Fixes kubernetes/kubernetes#8976

Special notes for your reviewer:
With existing usage of spew.Sprintf, the spew.Sdump is a better choice because it has the pretty format with full newlines, indentation and type.

Release note:

[k8s-ci-robot]

Welcome @haoruan!

It looks like this is your first PR to kubernetes/utils 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/utils has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[apelisse]

The original issue is 6 years old, and I don't know how much sense that makes today, is this still something we want? @thockin

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[dims]

/assign @thockin

[thockin]

Thanks for picking this up! Unfortunately, I don't think this quite meets needs.

https://go.dev/play/p/Gvyo85V6YdA

First, %+v and %#v are not "pretty". They are the opposite :)

Second, this doesn't disable methods, so somewhat less useful (would need this covered in test): https://go.dev/play/p/qciduo52P1q

Compare to litter (spew has more config, so does kr/pretty): https://go.dev/play/p/Z4Mz93Ip6Su

So I think the first step is to go look at k/k and see the places this would be used and figure out what most of them want. Then come back to this and explain why whichever impl you think is right is the best choice. Lastly, a rich test with maps, String() and Error() methods, etc

[haoruan]

Thanks the advise, seems we're not limited by the format/lib, I will check again

[haoruan]

spew.Sdump() has the pretty format with full newlines, indentation, type, and pointer information:

(main.Foo) {
 unexportedField: (*main.Bar)(0xf84002e210)({
  flag: (main.Flag) flagTwo,
  data: (uintptr) <nil>
 }),
 ExportedField: (map[interface {}]interface {}) {
  (string) "one": (bool) true
 }
}

Also we can set Config.DisableMethods to disable String(), or Error().

spew.Sdump() has been widely used in kubernetes to dump object, or show the output diff in failed unit test, and spew.Sprintf() did the same thing with the same purpose. I'm not sure if they're reserved by purpose or we just missed them.

So my opinion is to make it uniform across the whole project, and replace Sprintf() with Sdump() instead of creating a wrapper. Later we can close the original issue, and create a new one to do the replacement.

[thockin]

The reason I opened the issue was because it's all over the map. Most places that want to dump something don't ever look at the logs, so don't notice that it uses .String() or prints pointer values or whatever.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[haoruan]

/remove-lifecycle rotten

[thockin]

I realized this never got completed, sorry.

I don't love Sdump(), but it seems like least bad. If we're going to do this, we should add DisableCapacities. Spew seems unmaintained, maybe I'll fork it :)

We should probably call this package "dump" or "pretty" or similar.

We should document that the output of pretty.Print() may change over time and if anyone needs guaranteed output, they should take more direct control

[thockin]

A nice end-goal of this might be that nothing in k/k depends on spew directly.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haoruan
Once this PR has been reviewed and has the lgtm label, please ask for approval from thockin by writing /assign @thockin in a comment. For more information see:The Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[haoruan]

Thanks for the comments, PTAL if we're going to take it.

[thockin]

@BenTheElder @jpbetz @tallclair @cici37 @msau42 As discussed earlier, I recall now why JSON is not a great choice.

The GO JSON lib is very ...limited.

https://go.dev/play/p/ef_FY4KyrV-

Spew is unmaintained and large. I am sure we could fork it and make it do what we want but....ugh

kr.Pretty is already used trasitively, though I can't see how to make it's output not include newlines.

litter is nicer output, but is a net new dep. If we could get 3 or 4 deps which use spew to switch, we could justify this as a one-for-one.

@haoruan if you are still up for it, how about this:

1. move this kubernetes/kubernetes/pkg/util
2. convert all callsites in k/k to use this new function (same PR)
   == We should have only one place that uses spew
3. we work with a few deps to kill off spew
4. we convert it to litter or something else
   == No deps on spew
5. Move to kubernetes/utils

What do you think?

[haoruan]

Sounds good, will work on k/k first. Should we close this one?

[thockin]

yeah, let's do that, but please xref it so I don't forget :)

…

On Sun, Oct 16, 2022 at 8:14 PM Hao Ruan ***@***.***> wrote: Sounds good, will work on k/k first. Should we close this one? — Reply to this email directly, view it on GitHub <#226 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVDX4JDWU7YEKSLDRATWDTABBANCNFSM5HEAKMCQ> . You are receiving this because you were assigned.Message ID: ***@***.***>