About Monkey patch ruby for net/http added verbosity in #13745

[san983]

Description

Since 18.3.0 merging and releasing #13745, these outputs started showing up as part of the monkey patch added in the given PR. Perhaps the puts should be moved to Chef::Log.info or Chef::Log.debug?

Chef Version

18.3.0

Platform Version

#13745

Replication Case

Just running chef-client in any affected env/platform

Client Output

$ sudo chef-client --format min
Chef Infra Client, version 18.3.0
Patents: https://www.chef.io/patents
Infra Phase starting
opening connection to chef.example.com:443...
opened
starting SSL for chef.example.com:443...
SSL established, protocol: TLSv1.2, cipher: ECDHE-ECDSA-AES256-GCM-SHA384
opening connection to chef.example.com:443...
opened
starting SSL for chef.example.com:443...
SSL established, protocol: TLSv1.2, cipher: ECDHE-ECDSA-AES256-GCM-SHA384
opening connection to chef.example.com:443...
opened
...

Stacktrace

N/A

[Tensibai]

I vote for Chef::Log.debug as in the net/http gem these are debug logs https://github.com/ruby/ruby/blob/v3_1_2/lib/net/http.rb#L1702

[8la]

I vote for Chef:Log.debug, we will do a downgrade on our clients for a while, in order to make the debug process less annoying ; which truly is, with this noise around the logs .

Hope you can fix this soon.

Thanks!

[tpowell-progress]

This will catch the next Chef 18 promotion to stable. Still wrapping up Windows building issue but it will also be in any "current" channel release >= 18.3.0

[jkimalane]

Any news about removing those "puts" statements from the net/http monkey patch?

[Stromweld]

This should be fixed in the next 18.4.x release that should be coming soon.

[jkimalane]

You should really focus more on QA as this issue is not the first one, which requires monkey patching a monkey patch

[tpowell-progress]

You should really focus more on QA as this issue is not the first one, which requires monkey patching a monkey patch

Agreed, but that's an 🌲 problem in software engineering, compounded by the fact that there 20+ OS+architecture builds for each Chef version and that, while extremely annoying, extraneous output to STDOUT is not in itself a functional breakage (and even in manual testing I can identify plenty of times where such obvious errors have slapped me in the face but I had tunnel vision on a different problem).

Perhaps we need a scaffolding to validate output... or maybe even fail automated tests if STDOUT.puts is called? I think something like expect(STDOUT).not_to receive(:puts) could do that, but it would have to be only for a subset of test cases, since we have legitimate uses of STDOUT.puts in shell and some other areas.

Open to ideas from the community to detect similar classes of errors.

[Tensibai]

@jkimalane I find your comment a bit harsh in its wording.

@tpowell-progress naive idea: have a dumb run with a simple file resource against zero server and expect its stdout to exactly match something curated by a human (excluding the first line with the client version which couldn't match obvisouly). This should catch any extraneous output but will likely need update when wording change somewhere in the codebase but should catch unwanted things. I guess this could be extended to solo without zero with and withou dry-run enabled.

For the record a workaround is that you can dump the fixed file in the linked PR in a "temp-fix" cookbook libraries directory to cut the extra output to only early calls (before the cookbook is loaded) for test purposes and have a recipe with a file resource updating the 18.4 client file in place while waiting for the next release.

Let me know if this is something getting traction, I'll author the cookbook if needed.

[tpowell-progress]

@Tensibai I think breakage on wording changes is acceptable, as long as the test failure and description itself indicates that wording changes need to be checked as possible "false positives"

[gene1wood]

I can confirm this is fixed in 18.4.12 from #13970
Can this issue be closed out now?