Allow pre-commit to run in subpath

[LecrisUT]

Took some mucking around, but I've finally got something half-decent approach. It almost works other than #2006. Here is an example for testing:

  - repo: https://github.com/LecrisUT/tmt.git
    rev: pre-commit
    hooks:
      - id: tmt-lint
        args:
          - "--root"
          - "path/to/fmf"

Fixes: #2004

[lukaszachy]

/packit test

[LecrisUT]

Well at least the tests are happy so far, although there are no tests written for this. Can I leave writing these tests to the others? I'll just push a quick fix for RHEL8 builds when the tests are done.

Ok so pre-commit test failed 🤔

[15:05:25] [W] [worker_3] [stderr] [CentOS-Stream-9:x86_64:/plans/features/advanced]             00:00:27 fail /tests/precommit [7/16]

[LecrisUT]

@lukaszachy I will need some help on this. Locally when I test I get appropriate failures, but tmt seems to detect a fmf root from somewhere. And it is not like I am using #2008 here. Is it because I am not using local file url for the context url?

[LecrisUT]

/packit test

[LecrisUT]

@happz There are no other PRs that could help with the tests for this right? The failed test is very bizarre to me, i.e. where is the fmf root detected over there?

[happz]

@happz There are no other PRs that could help with the tests for this right? The failed test is very bizarre to me, i.e. where is the fmf root detected over there?

I'm not aware of any lint-related PRs other than #1851.

WRT the failed test, no idea, TBH, it's the first time I see the pre-commit hooks in action. Maybe @lukaszachy could help.

[LecrisUT]

Anyway, I have added the tests for the fmf_root. Can someone trigger the tests again?

[lukaszachy]

/packit test

[lukaszachy]

I'm sorry, not that much time for pre-commit stuff.

The idea behind that test is that it is common to forgot to include .fmf/version in the commit and then be suprised why no plans or tests are detected.

The test assumes fmf root == git root. For some use cases this is required (e.g. fedora/centos where plans are detected from git root) for others (e.g. this PR) not so much.

So we will need additional test to check that pre-commit finds issues if fmf-root option is used and no such root exists.

[LecrisUT]

Makes sense, so the issue is that my wrapper here still detects the current path's .fmf/version even though it is not added. I don't get how the the previous version checked that:

git ls-files --error-unmatch $(python3 -c 'import tmt; print(tmt.Tree(logger=tmt.Logger.create(), path=\".\").root)')/.fmf/version && tmt lint --source $@"

Maybe I just run git ls-files within the wrapper relative to that path will work?

[lukaszachy]

Makes sense, so the issue is that my wrapper here still detects the current path's .fmf/version even though it is not added. I don't get how the the previous version checked that:

git ls-files --error-unmatch $(python3 -c 'import tmt; print(tmt.Tree(logger=tmt.Logger.create(), path=\".\").root)')/.fmf/version && tmt lint --source $@"

The python script outpus absolute path to the fmf root so git ls-files --error-unmatch /path/.fmf/version is called. Error exit code means the file is not recognized.

Maybe I just run git ls-files within the wrapper relative to that path will work?

I'm afraid that it can be false positive - If the 'path' is set to say gitroot/foo/bar but fmf root is only in gitroot/foo the fmf.Tree call will find and report the gitroot/foo and the check might pass. However what the pre-commit should check instead is that the gitroot/foo/bar is known to git.

But we might have easier life: Use git rev-parse --show-toplevel to get absolute path to the git root and
call git ls-files --error-unmatch $(git rev-parse --show-toplevel)/$path/.fmf/version ? With 'path' empty (the default) it will keep checking for fmfroot==gitroot. With 'path' set it will make sure the supplied path is really fmf root

[LecrisUT]

I'm afraid that it can be false positive - If the 'path' is set to say gitroot/foo/bar but fmf root is only in gitroot/foo the fmf.Tree call will find and report the gitroot/foo and the check might pass. However what the pre-commit should check instead is that the gitroot/foo/bar is known to git.

I don't get this one, maybe we are missing each other point. What I was thinking of doing is effectively:

$ git ls-files ${root_path}/.fmf/version
$ tmt --root ${root_path} lint --source ...

(being wrapped into def pre_commit function and ${root_path} is the variable passed to the cli.)

But we might have easier life: Use git rev-parse --show-toplevel

Thanks I was just thinking about how to get git root to pass it there in the above.

[lukaszachy]

I don't get this one, maybe we are missing each other point. What I was thinking of doing is effectively:

$ git ls-files ${root_path}/.fmf/version
$ tmt --root ${root_path} lint --source ...

Cool, this should work. I got confused because in fmf id the 'path' is part between git root to the fmf root. Too many 'path's

[LecrisUT]

Ok, 7104337 should fix the issue. Am I missing anything else?

[LecrisUT]

/packit test

[LecrisUT]

Yey, tests are passing :)

[lukaszachy]

Just hide the --pre-check option, lgtm otherwise. It would be nice to have a dedicated tmt subcommand but as you mentioned it would require much more refactoring which is IMO not worth it.

[martinhoyer]

/packit build

[happz]

@LecrisUT when you have a moment, rebase, please, there seem to be conflicts.

[happz]

/packit build

[happz]

/packit build

[happz]

Unrelated issues reported by tests, as far as I can tell. And all conversations seem to be resolved, merging.