gitlint icon indicating copy to clipboard operation
gitlint copied to clipboard

Ignore stdin by default in CI

Open scop opened this issue 2 years ago • 7 comments

One of the nice things of nox are good defaults, no matter if it's run in CI or not.

I thought it'd make sense for gitlint, too. Following the nox implementation, something like this could do the trick:

--- a/gitlint-core/gitlint/config.py
+++ b/gitlint-core/gitlint/config.py
@@ -84 +84 @@ class LintConfig:
-        self._ignore_stdin = options.BoolOption("ignore-stdin", False, ignore_stdin_description)
+        self._ignore_stdin = options.BoolOption("ignore-stdin", "CI" in os.environ, ignore_stdin_description)

Perhaps some additional patching for the test suite would be in order, as it's likely to run also in CI itself, but in principle. Doc updates obviously too.

scop avatar Jan 31 '23 21:01 scop

We can do this yes (not sure whether this is the right implementation, need to check).

However, is the implication here that gitlint doesn't work out of the box in your CI environment without specifying --ignore-stdin? I haven't looked at TTY/STDIN detection for a while, but there have been other reports, mostly on windows IIRC (#91 and #171). Would like to hear more...

jorisroovers avatar Feb 01 '23 11:02 jorisroovers

That's what concerns me as I haven't needed to ignore stdin at all

sigmavirus24 avatar Feb 01 '23 11:02 sigmavirus24

I haven't come across a case where I would have actually needed it, rather just decided to follow gitlint's documentation on it being useful in (some?) CI :)

scop avatar Feb 02 '23 20:02 scop

So this should really be "improve documentation to indicate that this might be useful in CI"?

sigmavirus24 avatar Feb 03 '23 00:02 sigmavirus24

Would be good to be more specific than that. I'm not sure what audience would such a change serve, and there wouldn't be much of a difference to the current situation. Alternatively, the note about CI could be removed altogether, I suppose people who need it will either find the option or not, no matter if CI is mentioned.

scop avatar Feb 05 '23 17:02 scop

Sorry for the delay - and thanks for opening issues and your doc fix PRs @scop, please keep them coming. We might not always move forward with everything, but the discussion and suggestions make gitlint better overall - greatly appreciated!

On topic:

So if we don't know of any CI environment where detection fails, I think I'm now undecided on the usefulness of this.

I agree with using sane defaults ("batteries included" ftw!) but you could also argue it's better to have a hard fail on TTY/STDIN detection (and people opening a bug) than it silently being bypassed because CI was set.

Alternatively, the note about CI could be removed altogether, I suppose people who need it will either find the option or not, no matter if CI is mentioned.

Are you referring to the mention of CI here, in the CLI flag? Or do you mean elsewhere in the gitlint docs? https://github.com/jorisroovers/gitlint/blob/1f8a16b462e03878fdd7b9cbaaed506c4bf05956/gitlint-core/gitlint/cli.py#L270

jorisroovers avatar Mar 09 '23 09:03 jorisroovers

Both. There's a mention at least in docs/configuration.md (marked "sometimes useful" there).

scop avatar Mar 10 '23 18:03 scop