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

Restore code quality tooling #240

Closed
wants to merge 31 commits into from
Closed

Conversation

jonct
Copy link
Contributor

@jonct jonct commented Jul 17, 2024

This reverts commit 962b238.

I'll iterate on stage two of the plan here, specifically its contact surface with GitHub Actions.

@jonct jonct marked this pull request as ready for review July 18, 2024 04:02
@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 18, 2024

If hatch is used to build the zipapp for release then this binary should also be the one used during the integration test. Otherwise we won't know if hatch produced a broken binary and may release it in broken state.

Nice job on adding a few unit tests!

For properly testing get_mount_point I think we need to mock os.path.ismount. If we have a successful test we have an answer to this question too.

P.S. This time a single toml file is enough?

@jonct
Copy link
Contributor Author

jonct commented Jul 18, 2024

If hatch is used to build the zipapp for release then this binary should also be the one used during the integration test.

💯 Just trying to stay clear of each of your boundaries until my experiment proves adoptable.

For properly testing get_mount_point I think we need to mock os.path.ismount.

Ooo! Cool solution. 😋

P.S. This time a single toml file is enough?

Trying my level best to be responsive. If this was an issue for you, I'd hope you would let me know where and how. Then we could find our way together.

FYI building new custom plugins from scratch was some seriously vulnerable R&D, based entirely on being told that you wanted to avoid external dependencies. If I'd even thought you were cool with pulling in hatch-build-scripts for this, that would have saved me A LOT of time and effort to please you. 🕊️

Personally, I do like the external script better. I'd like to see it in src/scripts but didn't get the imports right in my first five tries. 🤪

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 19, 2024

Trying my level best to be responsive.

Much appreciated 🙂

I'm still a bit on the fence about putting Hatch in the critical path of building and releasing the app. I'm not familiar with it. But then again I'm also not going to maintain jailmaker in the future. I suppose the choice would be best made by a future maintainer.

Did you know you could create the jlmkr zipapp executable with the 10 line head preamble with just a single line of bash? If the preamble isn't in a text file it's these 2 lines of bash:

read -r -d '' PREAMBLE <<'EOF'
/usr/bin/env python3

jlmkr x.x.x

Persistent Linux 'jails' on TrueNAS SCALE to install software (k3s, docker, portainer, podman, etc.) with full access to all files via bind mounts.
SPDX-FileCopyrightText: © 2024 Jip-Hop and the Jailmakers <https://github.com/Jip-Hop/jailmaker>
SPDX-License-Identifier: LGPL-3.0-only


-=-=-=- this is a zip file -=-=-=- what follows is binary -=-=-=-
EOF

python3 -m zipapp src/jlmkr -p "$PREAMBLE" -o jlmkr
head jlmkr # Will show the first 10 lines

This could be invoked from Python to substitute the version variable. Seems simpler and less code than the Hatch build scripts and doesn't put Hatch in the critical path.

Python natively supports unit tests as well... Which leaves just the linting as value proposition? I do agree a commit hook for linting would be useful. Ruff has some documentation about this and has a GitHub action too.

@jonct
Copy link
Contributor Author

jonct commented Jul 19, 2024

I'm still a bit on the fence about putting Hatch in the critical path of building and releasing the app. I'm not familiar with it. But then again I'm also not going to maintain jailmaker in the future. I suppose the choice would be best made by a future maintainer.

Python natively supports unit tests as well... Which leaves just the linting as value proposition? I do agree a commit hook for linting would be useful. Ruff has some documentation about this and has a GitHub action too.

Right. It's an adjustment, and I didn't loop you in on the full details of survey and selection. 😢 We jumped pretty quickly from having me make my case through insufferable blather to having me make my case through careful legwork and turnkey delivery.

Please allow me to back up and reintroduce…

All that Hatch does — and it's significant — is to tie together all of those same tools you're thinking of. Ruff, PyTest, coverage, pycodestyle, pydocstyle… probably a bunch of others I don't even know to look for. The tools you've enjoyed privately through your choice of IDE plugins, now being discreetly but consistently applied for all contributors and PRs.

Hatch preps, configures, and runs all of these in their own safe out-of-the-way sub-environments so we don't have to.

Because we don't want to.

We'd rather just code. Maybe let a robot help assess contributions and watch for regressions. Without getting mired down maintaining a dozen other robots and their metadata individually.

That should prove itself valuable to you as owner-maintainer. It is plainly indispensable to the project if you step away.

Did you know you could create the jlmkr zipapp executable with the 10 line head preamble with just a single line of bash? If the preamble isn't in a text file it's these 2 lines of bash:

This could be invoked from Python to substitute the version variable. Seems simpler and less code than the Hatch build scripts and doesn't put Hatch in the critical path.

Wait — do you not like build.py either?! 😣

@jonct jonct marked this pull request as draft August 8, 2024 08:31
@jonct jonct marked this pull request as ready for review August 9, 2024 01:29
@jonct
Copy link
Contributor Author

jonct commented Aug 9, 2024

I think this strikes the balance that the three of us were triangulating toward. 🙏

Including the ability to develop and test in place before building and chmodding.

And performing integration tests in CI, using the exact delivered artifacts.

If this is acceptable, then we can merge and start into meaningful unit tests.

@jonct
Copy link
Contributor Author

jonct commented Aug 9, 2024

No one asked me for this, but FWIW here's my take on a harness/runner for small-grained integration tests.

python3 -m scripts.test

This will execute each scripts/test_* inside its own private temp ZFS pool, and tally up the results.

Tests can be written in any language. Each one just wakes up in its own Jailmaker directory alongside its own jlmkr tool, and does whatever's necessary. If it returns any status code but zero, it's considered to have failed.

Note that running it locally will expose your system to any lingering side-effects of the tests themselves — including network changes and other systemd units. But the temporary ZFS pools clean up and go away on their own.

The GitHub runner environment has no such concerns. It's nice to see the ZFS code running there.

@Jip-Hop Jip-Hop closed this Nov 1, 2024
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

Successfully merging this pull request may close these issues.

2 participants