.format syntax is not supported on python 2.4. Even if this codepath will not be used on these builders, python will fail to compile it.

@tkopecek thanks for the review!

I haven't touched a Python that ancient for what must be a decade now. Nonetheless, I've revised the offending code to use % style formatting.

It works for me now.

It looks good to me.

@ngompa thanks for the review!

I've also tested this against the new 1.11 and things still look good here.

I just force pushed an update. One of my rebase ops somehow wound up having the new CLI opts tied to the spin-livecd sub-command rather than spin-livemedia as it was originally. Otherwise, there were no other changes.

Is there any strong argument for eliminating the --lorax-dir option and instead requiring that value via --lorax-url in a manner similar to how it's done with --ksurl? Specifically, using a format like:

scheme://host/path_to_repo?path_to_templates_in_repo#revision_or_tag

I was thinking I'd seen both approaches in the CLI and it was while looking for that evidence that I noticed the mistake addressed in my prior comment. Anyway, I think consistency best, but I now fail to find the example I vaguely recall referring to when I went with two separate options.

I was hoping to see this merged in the next Koji release, but it looks like it's going to miss 1.12. I've been using it (on top of 1.11) successfully on a daily basis for a few months now.

Is there anything I can do to help move this along?

@jflorian Rebasing it to master would help. :)

@ngompa I did that several times before already. I'm willing to do so again, but only if it isn't futile. Is there an ideal time to do so... say just before a release is readied so I needn't waste effort? (I didn't know 1.12 was being cut until it was too late.)

@jflorian For what it's worth, I didn't know until now either. @mikem, can we please try to pull this into 1.12?

Sorry, I haven't had time to take a look at this. We're pretty much set on 1.12, but we're aiming for a more frequent releases, so 1.13 should not be terribly far out.

rebased here: https://github.com/mikem23/koji-playground/tree/lorax-templates

please follow pep8 guidelines for docstrings in new code

maybe we can come up with a different name for this to make it a little clearer what it is?
I mean the result is not a relative path, it's and absolute path within the buildroot.

use koji.util.relpath

ditto pep8

that said, thank you for the detailed docstrings. they just need a short summary :)

Is there a reasonable default we might use here?

@mikem Given the comment about summaries, I assume you mean PEP257, not PEP8. To the best of my knowledge, all of my docstrings are already PEP8 compliant. I apologize for the dreadful relpath method name, I'd given it much thought and never arrived at anything to my liking and just pushed on to focus on the goal, intending to come back and give it more attention later... and then simply forgot about it. My develop2 branch addresses all of the above, hopefully with exception of a default for --lorax_dir for which I'm not sure what would serve as a good value. I once had this as templates.d, but myself ran into an issue where simply having a copy of that directory from my lorax-templates-generic-25 install wasn't sufficient. I had to move everything under 99-generic/ up one level and rmdir 99-generic. Consequently, I thought it might be misleading to suggest templates.d as a default for fear it might lead others to also assume that should work.

Anyway, please look at my develop2 branch and let me know what you think. This is a rebase of master. For some reason git won't let me do a clean push of develop without either a force or some additional merge work, the necessity of which has me confused. I was afraid that forcing the push might "wreck" this PR somehow given my limited Pagure experience.

PEP257, not PEP8. To the best of my knowledge, all of my docstrings are already PEP8 compliant.

PEP8 refers to PEP257 in its docstring section, and rolls off my memory easier, but you're right, I should be more specific. I guess I always read that section of PEP8 to essentially #include PEP257.

I apologize for the dreadful relpath method name, I'd given it much thought and never arrived at anything to my liking

Perhaps rootpath, inrootpath, brpath, buildrootpath. None of these are great, but I'd err on clarity over style. I think I might prefer inrootpath out of these four.

with exception of a default for --lorax_dir for which I'm not sure what would serve as a good value

Seems a little odd that we require two options, while the base command requires only one. I mean, I get it, the base command isn't fetching from an scm.

Does the location of the dir affect the image in any way? I'd assume just the contents of it. Why require the user to pick an arbitrary name for what amounts to a temporary directory?

For some reason git won't let me do a clean push of develop without either a force or some additional merge work, the necessity of which has me confused. I was afraid that forcing the push might "wreck" this PR somehow given my limited Pagure experience.

The PR is tied to the branch it is based on. Change the branch and you change the PR. If it's just a fast-forward then the PR will show the new commits. If it's not a fast-forward, then the PR will indicate that the changes were "rebased." If you've rebased, you'll have to use the force option to push. Won't hurt the PR, but it will of course change it.

For this reason, it's best practice to make a new branch for each PR and give it a descriptive name. e.g. "lorax-templates" . Names like develop and develop2 sounds like branches where one might place many different changes.

PEP257, not PEP8. To the best of my knowledge, all of my docstrings are already PEP8 compliant.

PEP8 refers to PEP257 in its docstring section, and rolls off my memory easier, but you're right, I should be more specific. I guess I always read that section of PEP8 to essentially #include PEP257.

Fair enough. I was caught a bit off guard because I do all my editing via PyCharm and have it set to be very strict WRT to PEP8 ... which makes most of the koji code "glow" with warnings. :-)

I apologize for the dreadful relpath method name, I'd given it much thought and never arrived at anything to my liking

Perhaps rootpath, inrootpath, brpath, buildrootpath. None of these are great, but I'd err on clarity over style. I think I might prefer inrootpath out of these four.

You didn't mention what you thought of my new name, path_without_to_within. While it's a mouthful, I think it provides clarity, especially in conveying that there's a conversion going on here.

with exception of a default for --lorax_dir for which I'm not sure what would serve as a good value

Seems a little odd that we require two options, while the base command requires only one. I mean, I get it, the base command isn't fetching from an scm.
Does the location of the dir affect the image in any way? I'd assume just the contents of it. Why require the user to pick an arbitrary name for what amounts to a temporary directory?

You're right that it's the content that matters, but I don't understand what you mean by stating it's a temporary directory. To be clear, --lorax_dir specifies where in the clone that templates may be found, not the name of the directory containing the clone. (In my use case, the templates are only a fraction of the content.)

For some reason git won't let me do a clean push of develop without either a force or some additional merge work, the necessity of which has me confused. I was afraid that forcing the push might "wreck" this PR somehow given my limited Pagure experience.

The PR is tied to the branch it is based on. Change the branch and you change the PR. If it's just a fast-forward then the PR will show the new commits. If it's not a fast-forward, then the PR will indicate that the changes were "rebased." If you've rebased, you'll have to use the force option to push. Won't hurt the PR, but it will of course change it.

Thanks for the info. I probably already discovered this once, but wanted to be more careful now that this PR has some history tied to it.

For this reason, it's best practice to make a new branch for each PR and give it a descriptive name. e.g. "lorax-templates" . Names like develop and develop2 sounds like branches where one might place many different changes.

Sorry about that; it's a mere reflection of how I need to keep things straight on my end keeping changes like these separate from those that shouldn't be shared (e.g., our own "master" branch that reflects something akin to koji-1.11 with these patches (because my job depends on them) and others I had to cherry-pick to keep us alive in the interim.) Generally, I'd use more topic-specific names.

So, if I make new PRs for effectively the same feature (or bug, whatever), that means Pagure sort of requires a broken history? Like, "this PR has been closed/obsoleted by NEW_NUMBER"? If so, that (fragmented history) seems kind of icky.

Please see PR#419 which supersedes this PR.