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

pre-commit fmf subpath #2004

Closed
LecrisUT opened this issue Apr 18, 2023 · 13 comments · Fixed by #2007
Closed

pre-commit fmf subpath #2004

LecrisUT opened this issue Apr 18, 2023 · 13 comments · Fixed by #2007
Labels
command | lint tmt lint command good first issue Good for newcomers
Milestone

Comments

@LecrisUT
Copy link
Contributor

Allow to run pre-commit with fmf root in a subpath

@lukaszachy
Copy link
Collaborator

This means we need a way to propagate --root for tmt commands and for path= argument used in git ls-files

@lukaszachy lukaszachy added command | lint tmt lint command enhancement good first issue Good for newcomers labels Apr 18, 2023
@lukaszachy
Copy link
Collaborator

Might be easy enough. Docs to read about hooks: https://pre-commit.com/#new-hooks

@LecrisUT
Copy link
Contributor Author

Indeed, if it was such a simple fix, I would have made a PR with the feature. If we can create a simple wrapper I think it's trivial, but otherwise I don't see how to extract a specific args in the .pre-commit-hooks.yaml format. Do you think it worth making a small wrapper file for that?

@lukaszachy
Copy link
Collaborator

I'm afraid we will have to. If tmt could accept args everywhere it would be easy but it doesn't (we need to add it as the first option)....
The 'entry point' with current 'bash -c' is too hackish to my liking anyway, dedicated scripts might be easier to fix/extend .

@LecrisUT
Copy link
Contributor Author

I did a bit of quick testing, and there is a more crucial issue that tmt --root path/to/fmf_root lint fails:

fail No tree found in repo 'None', missing an '.fmf' directory?

Running the tmt --root ... run works though so this is a bug in lint.

@lukaszachy
Copy link
Collaborator

Which tmt version is that?

$ tmt --version
tmt version: 1.22.0 (ea71a188)

$ tree -a
.
└── a
    ├── .fmf
    │   └── version
    └── t.fmf

3 directories, 2 files

$ tmt t lint

No metadata found in the '.' directory. Use 'tmt init' to get started.

$ tmt --root a t lint
/t
pass test script must be defined
pass directory path must be absolute
pass directory path must exist
warn summary is very useful for quick inspection
pass correct attributes are used

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Apr 19, 2023

I have tried with both 1.21 and 1.22. Try with this minimal example plan and with a git repo:

summary:
  simple example
discover:
  how: fmf
  filter: "tag: something"
execute:
  how: tmt

@LecrisUT
Copy link
Contributor Author

@lukaszachy Let me know if you can confirm this behaviour

@lukaszachy
Copy link
Collaborator

Confirmed. Please create the issue for lint

@LecrisUT
Copy link
Contributor Author

Ok, separated in #2006

@LecrisUT
Copy link
Contributor Author

LecrisUT commented Apr 19, 2023

As for the pre-commit, I've tried a hack as:

-   id: tmt
    name: tmt
    entry: tmt
    files: '.*\.fmf$'
    verbose: true
    pass_filenames: true
    language: python
    language_version: python3

and

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

It sort-of works, but it is not a good solution. We should still maybe make different executable that is not installed outside of a pypi environment (or maybe not even there?). Is it ok if this one is not using click and entry-point for this one?

Also, what do you think about modernizing the build to PEP621 (requires dropping python 3.6 support and rhel8 at least and switching the versioning to git tags)

@lukaszachy
Copy link
Collaborator

I'd say rhel-8 support (so python 3.6) is required for now and we will start thinking about being python3.9+ once centos-8 is EOL or rhel-8 moves to AUS

@LecrisUT If you find a way how to create helper script just for the pre-commit usage it would be nice.
All I can think of now is to create separate project (e.g. teemtee/precommit) and let it live there.

@LecrisUT
Copy link
Contributor Author

Yeah, I've managed to make a helper script in #2007 as you can see. So far works ok, just missing tests and this test if failing. Might be something deeply rooted in testing-farm, so I don't know how to deal with it.

@psss psss added this to the 1.34 milestone May 6, 2024
@happz happz modified the milestones: 1.34, 1.35 Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command | lint tmt lint command good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants