[mozesl-nokia]

Proof of concept for the printer rewrite so we can get the conversation going.

Nothing is wired in yet and the previous printer is still in place. The new one is aptly named Printer2 for now. Of course, the goal is to replace the original in the final version.

Fixes #4698

See also #4724

[netlify]

✅ Deploy Preview for kptdocs ready!

Name	Link
🔨 Latest commit	f82b145
🔍 Latest deploy log	https://app.netlify.com/projects/kptdocs/deploys/6ada05b0d47e6077fcad5ccc
😎 Deploy Preview	https://deploy-preview-4779--kptdocs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[mozesl-nokia]

@SurbhiAgarwal1 could you take a look?

[SurbhiAgarwal1]

Hi @mozesl-nokia, thank you for putting this PoC together!

I took a look through Printer2 and the logger abstractions. I really like the separation with MapPrinter / StructuredPrinter and the builder workflow it makes the API feel very clean.

I had a few quick questions and thoughts while reading through the code to make sure I understand your vision:

1. StdLogger.PrintM key ordering & destination:

   • Since Go map iteration is randomized, I was wondering if we should sort the keys before printing in StdLogger.PrintM (or use slice-based key-values) so the CLI output order stays consistent for users and automated tests?
   • Also, would it make sense for StdLogger to accept an io.Writer (defaulting to os.Stdout) in case callers need to redirect output to os.Stderr or capture it in tests?

2. Builder defaults and copying:

   • In WithArg(), would it be safer to call EnsureDefaults() there as well, just in case someone initializes a builder directly with &Builder{}?
   • In Build(), do you think we should do maps.Clone(p.Args) so each built printer holds its own independent copy of the args?

3. Multiline outputs (re: line 126 TODO):

   • Regarding your TODO about printFnResult, printFnExecErr, and printFnStderr: for structured loggers (klog/slog), do you think treating multiline output as a single string field (e.g. "output": stderr) makes sense? And for human CLI (StdLogger), printing the event header first and then indenting the raw multiline output below it? Happy to hear your thoughts on what format you prefer.

4. Formatting in api/fnresult/v1/errors.go:

   • In ExecError.String(), I noticed that Exit Code: might end up on the same line as the last stderr entry. Was that intended, or should we add a newline before the exit code?
   • Also, is the TruncateOutput logic planned for a follow-up commit?

Happy to help write unit tests or help wire this into render_executor whenever you're ready. Let me know what you think!

[mozesl-nokia]

@SurbhiAgarwal1

1. Keys are now ordered, replaced StdLogger with IOLogger
2. Made the builder type private, not that anybody outside of kpt and Porch should use it. I think that extra clone is not necessary.
3. In klog multiline usually looks wrong, but for the CLI it is much more readable. The args should probably be on a new line after a multiline print, and probably the logger object should be handling formatting/reformatting. (Maybe take in a lines ...string arg in a new PrintLines method or something?)
4. I noticed it too, it will probably have to be handled by the logger. It should go onto a new line in the CLI at least. I added the TruncateOutput field to the builder and the printer as well, I don't like the weird global flag we currently have.

[sonarqubecloud]

Quality Gate failed

Failed conditions
19.8% Coverage on New Code (required ≥ 80%)
B Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE