Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore stdin by default in CI #423

Open
scop opened this issue Jan 31, 2023 · 7 comments
Open

Ignore stdin by default in CI #423

scop opened this issue Jan 31, 2023 · 7 comments
Milestone

Comments

@scop
Copy link
Contributor

scop commented Jan 31, 2023

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.

@jorisroovers
Copy link
Owner

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...

@sigmavirus24
Copy link
Collaborator

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

@scop
Copy link
Contributor Author

scop commented Feb 2, 2023

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 :)

@sigmavirus24
Copy link
Collaborator

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

@scop
Copy link
Contributor Author

scop commented Feb 5, 2023

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.

@jorisroovers
Copy link
Owner

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?

help="Ignore any stdin data. Useful for running in CI server.")

@scop
Copy link
Contributor Author

scop commented Mar 10, 2023

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

@jorisroovers jorisroovers added this to the 0.20.0 milestone Apr 11, 2023
scop added a commit to scop/pytekukko that referenced this issue May 4, 2023
I'm not aware of a setup where it would be actually required.

Refs jorisroovers/gitlint#423
scop added a commit to scop/home-assistant-jatekukko that referenced this issue May 4, 2023
I'm not aware of a setup where it would be actually required.

Refs jorisroovers/gitlint#423
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants