Fail Rubocop on warn logging

[aduth]

🛠 Summary of changes

Updates Rubocop to fail the build if there are any warning messages emitted.

Rubocop may warn when there's incorrect usage, but it doesn't trigger a failure. The changes here override Kernel#warn in the context of Rubocop linting to exit with a non-zero exit code.

📜 Testing Plan

Prior to fix, verify that the build fails, including an error message. (Edit: See https://gitlab.login.gov/lg/identity-idp/-/jobs/1513614)

Example:

Error: Unexpected warn logging occurred:

lib/identity_config.rb: Metrics/LineLength has the wrong namespace - replace it with Layout/LineLength

[aduth]

I think this is failing as expected: https://gitlab.login.gov/lg/identity-idp/-/jobs/1513614

Though it also includes an additional warning I wasn't expecting about Parallel::DeadWorker

Error: Unexpected warn logging occurred:
lib/identity_config.rb: Metrics/LineLength has the wrong namespace - replace it with Layout/LineLength
Error: Unexpected warn logging occurred:
Parallel::DeadWorker

It might be a result of calling exit. Could be okay? A little distracting to have that included as well though.

Alternatively, could look at raising an exception, or curious if there's some equivalent of Node.js's process.exitCode assignment for Ruby (i.e. set the exit code, but don't end the process immediately). I wasn't able to find one. I experimented with raising an exception and it does work, but the output is much noisier since it includes a full stack trace, and the actual warning message is hard to find in the output.

[zachmargolis]

1. I might document this with a comment? Big change in behavior from typical Ruby
2. This seems like a lib/core_ext kinda file to me, I would move it there? Since lib is not autoloaded it should be safe

Suggested change

	module Kernel
	# Module that overrides Kernel#warn to immediately exit
	# Should NOT be included in normal web server mode, is only used in specific scripts
	module Kernel

[aduth]

Yeah, good point on both counts. I'll make those updates.

[lmgeorge]

I would rather we not do this. This is an extreme change to Ruby's default behavior.

Instead, can we:

1. Disable cache so that we always get the warning output and consume that output to change the exit code OR
2. Write a cop to detect this specific issue.

[aduth]

@lmgeorge What do you think of the updated approach? I think it's an improvement. I hadn't realized that these warnings were being emitted to stderr, so it becomes a test of "fail if there's any stderr".

Example build failure: https://gitlab.login.gov/lg/identity-idp/-/jobs/1515745

Example output:

--- rubocop ---
mkdir -p tmp
bundle exec rubocop --parallel --format progress --format junit --out rubocop.xml --display-only-failed --color 2> tmp/rubocop.txt
Inspecting 2259 files
.......
2259 files inspected, no offenses detected
[ -s tmp/rubocop.txt ] && (printf "Error: Unexpected stderr output from Rubocop\n"; cat tmp/rubocop.txt; exit 1)
Error: Unexpected stderr output from Rubocop
lib/identity_config.rb: Metrics/LineLength has the wrong namespace - replace it with Layout/LineLength
lib/identity_config.rb: Metrics/LineLength has the wrong namespace - replace it with Layout/LineLength
make: *** [Makefile:71: lint] Error 1

[mitchellhenke]

I sort of lean towards @lmgeorge's concerns on this in that the change feels pretty heavy (though limited in scope) for the upside. The only alternative I have is to grep the output for the message we don't expect to see. It's not quite the same though. I don't intend for this to be a blocking suggestion since anything going sideways on this will hopefully be pretty apparent.

[aduth]

@mitchellhenke I've since changed from the initial approach in recent commits.

[lmgeorge]

@aduth I can agree with this approach of grepping the stderr output.

My main concern is that it can be easy to see an existing pattern and replicate it elsewhere even when strong language in the form of doc comments warn (heh) against it. I think this new approach addresses that concern. Thank you for being willing to flex on this!

[lmgeorge]

I would rather we not do this. This is an extreme change to Ruby's default behavior.

Instead, can we:

1. Disable cache so that we always get the warning output and consume that output to change the exit code OR
2. Write a cop to detect this specific issue.

[matthinz]

I think I'm seeing a difference in how this behaves in CI. Basically, I have a job that is failing the lint phase, but the CI logs don't include the output from tmp/rubocop.txt.

I added the rubocop.txt file to the gitlab artifacts and downloaded it to verify that it is present. I think there might be a difference in how the make running inside the CI docker container behaves compared to the one installed on my laptop.

[aduth]

@matthinz Interesting, yeah. I think the tmp/rubocop.txt artifact points to the problem, but I would expect that to be output in the build log, which doesn't appear to be working as expected there.

Maybe it's because it doesn't actually get far enough to the awk command: a combination of the rubocop command exits with non-0 status code, but the stderr is being put into tmp/rubocop.txt instead.

Maybe we can tolerate the non-0 status code result , like throwing a || true somewhere in there? Then it'll still fail once it reaches the awk line, but at least output the failure reason.

[aduth]

Continuing issue with tmp/rubocop.txt output here: #11642