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

Split up jlmkr.py into smaller files as-is #233

Merged
merged 58 commits into from
Jul 16, 2024

Conversation

jonct
Copy link
Contributor

@jonct jonct commented Jul 13, 2024

Stage One of a three-stage modularization plan. See discussion.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 13, 2024

Did you try this?

https://github.com/Jip-Hop/udev-trigger-zfs-autobackup/blob/b24c26988f16cbdd2e9efd45d6e0ee7921e583cb/trigger.sh#L66

This used to work on SCALE but perhaps it no longer works...

@jonct
Copy link
Contributor Author

jonct commented Jul 13, 2024

Did you try this?

Heck yes. See my Jira.

I'd actually gotten a handful of commits into dissection using only an editor and a Makefile on macOS. But then doubled back to test launch, needed a ZFS dataset your other preconditions, and realized that I should get the final target build system in place first.

For the venv I exhausted a bunch of potential workarounds using pipx and such, then submitted NAS-130029. For orchestration I surveyed current practices, got up to speed on PEP 517/518, and circled in on Hatch to keep ceremony to a minimum. Since then I've been working on configuring that and the associated GitHub build action. After that I'll be mercifully back to parceling out the script again. Apologies for all the yak shaving.

@jonct
Copy link
Contributor Author

jonct commented Jul 15, 2024

That's the big one with all the moving parts. Now on to the Python script itself.

@jonct jonct requested a review from Jip-Hop July 15, 2024 23:59
@Jip-Hop Jip-Hop force-pushed the lift-and-separate branch 4 times, most recently from 839d45b to c72bfa2 Compare July 16, 2024 11:43
@Jip-Hop Jip-Hop force-pushed the lift-and-separate branch 2 times, most recently from 91e5f6b to d98ccf8 Compare July 16, 2024 11:52
@Jip-Hop Jip-Hop force-pushed the lift-and-separate branch from d98ccf8 to 1679d27 Compare July 16, 2024 11:56
Copy link
Owner

@Jip-Hop Jip-Hop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed some more imports and added a new entrypoint script so the tests can run again (running the test caught this import error). Although they are rudimentary let's strive to keep running the tests we have. From the last run I see the "jailmaker" dir safety check needs changes.

The modularization is a good first step but hasn't yet resulted in working code. Looking forward to seeing the tests green again. 😃

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 16, 2024

I'd like to keep it simple. For now no Hatch.

@Jip-Hop Jip-Hop merged commit f343ff8 into Jip-Hop:v3.0.0 Jul 16, 2024
1 check passed
@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 16, 2024

🥳

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 16, 2024

Please have a look at the updated README, specifically this bit about configuring the JAILMAKER_DIR via env variable. This user defined path would then be used instead of SCRIPT_DIR_PATH = os.path.dirname(SCRIPT_PATH). That would make it possible to separate the jlmkr zipapp from the location where the jails data is stored. Is that something you'd like to implement?

@jonct
Copy link
Contributor Author

jonct commented Jul 16, 2024

WOOOOOT! 🤩

Is that something you'd like to implement?

Yespleez. I, too, found this to be a linchpin to get tests back online. PR forthcoming.

FYI, I envision a fallback progression for which exact policy details will be ripe for discussion and adjustment.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Jul 16, 2024

For now I'd be happy with just the env var mechanics.

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