#233 Koji support for custom Lorax templates in LiveMedia tasks
Closed 2 years ago by jflorian. Opened 2 years ago by jflorian.
jflorian/koji develop  into  master

file modified
+42

@@ -693,6 +693,21 @@ 

              rpm_info['external_repo'] = erepo

              rpm_info['location'] = erepo['external_repo_id']

  

+     def relpath(self, path):
mikem commented 2 years ago

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.

+         """

+         :param path:
mikem commented 2 years ago

please follow pep8 guidelines for docstrings in new code

+             A reference within the BuildRoot but as an absolute path from

+             without a chroot.

+         :return:

+             The equivalent absolute path from within a chroot of the BuildRoot.

+         """

+         root = self.rootdir()

+         if os.path.commonprefix([root, path]) != root:

+             raise ValueError(

+                 'path %r is not within the BuildRoot at %r' % (path, root)

+             )

+         return os.path.join('/', os.path.relpath(path, root))
mikem commented 2 years ago

use koji.util.relpath

+ 

      def resultdir(self):

          return "%s/%s/result" % (self.options.mockdir, self.name)

  

@@ -2937,6 +2952,29 @@ 

      # https://bugzilla.redhat.com/show_bug.cgi?id=1315541

      bind_opts = {}

  

+     def fetch_lorax_templates_from_scm(self, build_root):

+         """

+         Make a checkout of the lorax templates from SCM so that they may be
mikem commented 2 years ago

ditto pep8

+         passed to livemedia-creator.  Here we are operating outside the chroot
mikem commented 2 years ago

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

+         of the BuildRoot.  The following options are essential:

+             - lorax_url points to the SCM containing the templates.

+             - lorax_dir provides a relative reference to the templates within

+             the checkout.

+ 

+         :param build_root:

+             The BuildRoot instance to receive the checkout.

+         :return:

+             An absolute path (from within the chroot) to where livemedia-creator

+             can find the checked out templates.

+         """

+         scm = SCM(self.opts['lorax_url'])

+         scm.assert_allowed(self.options.allowed_scms)

+         logfile = os.path.join(self.workdir, 'lorax-templates-checkout.log')

+         checkout_dir = scm.checkout(os.path.join(build_root.rootdir(), 'tmp'),

+                                     self.session, self.getUploadDir(), logfile)

+         return os.path.join(build_root.relpath(checkout_dir),

+                             self.opts['lorax_dir'])

+ 

      def genISOManifest(self, image, manifile):

          """

          Using iso9660 from pycdio, get the file manifest of the given image,

@@ -3050,6 +3088,10 @@ 

          if arch == 'x86_64':

              cmd.append('--macboot')

  

+         if 'lorax_url' in self.opts:

+             templates_dir = self.fetch_lorax_templates_from_scm(broot)

+             cmd.extend(['--lorax-templates', templates_dir])

+ 

  

          # Run livemedia-creator

          rv = broot.mock(['--cwd', '/tmp', '--chroot', '--'] + cmd)

file modified
+11 -1

@@ -5466,6 +5466,13 @@ 

          help=_("SCM URL to spec file fragment to use to generate wrapper RPMs"))

      parser.add_option("--skip-tag", action="store_true",

                        help=_("Do not attempt to tag package"))

+     parser.add_option('--lorax_dir', metavar='DIR',

+                       help=_('The relative path to the lorax templates '

+                              'directory within the checkout of "lorax_url".'))

+     parser.add_option('--lorax_url', metavar='URL',

+                       help=_('The URL to the SCM containing any custom lorax '

+                              'templates that are to be used to override the '

+                              'default templates.'))

      (task_options, args) = parser.parse_args(args)

  

      # Make sure the target and kickstart is specified.

@@ -5474,6 +5481,9 @@ 

                         " architecture, a build target, and a relative path to" +

                         " a kickstart file."))

          assert False  # pragma: no cover

+     if task_options.lorax_url is not None and task_options.lorax_dir is None:

+         parser.error(_('The "--lorax_url" option requires that "--lorax_dir" '

+                        'also be used.'))
mikem commented 2 years ago

Is there a reasonable default we might use here?

      _build_image(options, task_options, session, args, 'livemedia')

  

  # This handler is for spinning appliance images

@@ -5845,7 +5855,7 @@ 

      passthru_opts = [

          'isoname', 'ksurl', 'ksversion', 'scratch', 'repo',

          'release', 'skip_tag', 'vmem', 'vcpu', 'format', 'specfile',

-         'title', 'install_tree_url',

+         'title', 'install_tree_url', 'lorax_dir', 'lorax_url',

          ]

      hub_opts = {}

      for opt in passthru_opts:

This resolves https://bugzilla.redhat.com/show_bug.cgi?id=1394933 for me. I've tested this with a Fedora 25 client, koji-hub-1.10.1-10.el7.noarch and a Fedora 24 builder.

7 new commits added

  • Don't require protonmsg on rhel5
  • more README formatting
  • Fix README formatting now that pagure has enabled nl2br
  • kojiweb: [rpminfo] add tooltip for SIGMD5 field
  • cli: [rpminfo]rename "Payload" to "SIGMD5"
  • kojiweb: [rpminfo]rename "Payload Hash" to "SIGMD5"
  • don't bind-mount /dev for LiveMediaTask (RHBZ #1315541)
2 years ago

rebased

2 years ago

rebased

2 years ago

.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.

rebased

2 years ago

@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.

rebased

2 years ago

@ngompa thanks for the review!

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

2 new commits added

  • New - custom lorax templates for livemedia tasks
  • New - kojid.BuildRoot.relpath()
2 years ago

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.

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

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.

Pull-Request has been closed by jflorian

2 years ago
Metadata