Skip to content

Commit

Permalink
Ignore the old --squashfs-only argument with a null action
Browse files Browse the repository at this point in the history
01dd27a broke lorax due to some unspecified behavior in argparse
when two different arguments write to the same target. It added
a --rootfs-type argument with a string value and a default of
"squashfs", and changed --squashfs-only to write the value
"squashfs" to the same target (rootfs_type) if passed.

Unfortunately, if you do this *with --squashfs-only first and
--rootfs-type second*, then if neither --squashfs-only nor
--rootfs-type is passed, the value of rootfs_type in the
resulting args dict is None, not "squashfs" as expected.

If you reverse the order of the args - so --rootfs-type is first
and --squashfs-only is second - then if neither arg is passed,
the value of rootfs_type comes out as "squashfs" as expected.
So we *could* just swap the args in both places. However, this
strikes me as fragile as it seems like this behaviour is not
defined and could change unexpectedly.

It seems safer, to me, to accept --squashfs-only but simply
ignore it (since the default value of --rootfs-type is squashfs
anyway, and if someone were to specify both, I think it's sane
for --rootfs-type to 'win'). Weirdly, argparse doesn't seem to
have a built-in way to do this, so I had to invent a new "null"
action, but it's quite short.

Alternatively we could use parse_known_args instead of
parse_args and then check that the only unknown arg was
squashfs-only, but we'd have to do that in multiple places, this
seems a bit simpler.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
  • Loading branch information
AdamWill committed Jul 16, 2024
1 parent d358d23 commit 0193ff5
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 7 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/example-livemedia-creator.yml
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,12 @@ jobs:
## Create the ISO
- name: Create the custom ISO
# --no-virt: Needed since we're in a container, no host CPU
# --squashfs-only: Just to speed things up, not required
run: |
livemedia-creator \
--ks "${{ inputs.kickstart_path }}" \
--no-virt \
--make-iso \
--iso-only \
--squashfs-only \
--iso-name Fedora-custom-example.iso \
--project Fedora \
--volid "Fedora-${{ inputs.fedora_release }}" \
Expand Down
22 changes: 17 additions & 5 deletions src/pylorax/cmdline.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@

version = "{0}-{1}".format(os.path.basename(sys.argv[0]), vernum)

class NullAction(argparse.Action):
"""
An argparse Action which does nothing. Used to accept but
ignore the old --squashfs-only argument.
"""
def __init__(self, *args, **kwargs):
"""Minimal required to let the arg have no value."""
super().__init__(*args, **kwargs, nargs=0, default=None, required=False)
def __call__(self, *args, **kwargs):
"""Do nothing."""
pass


def lorax_parser(dracut_default=""):
""" Return the ArgumentParser for lorax"""

Expand Down Expand Up @@ -109,9 +122,8 @@ def lorax_parser(dracut_default=""):
help="Do not verify SSL certificates")
optional.add_argument("--dnfplugin", action="append", default=[], dest="dnfplugins",
help="Enable a DNF plugin by name/glob, or * to enable all of them.")
optional.add_argument("--squashfs-only", action="store_const", const="squashfs",
default="squashfs", dest="rootfs_type",
help="Use a plain squashfs filesystem for the runtime.")
optional.add_argument("--squashfs-only", action=NullAction,
help="Ignored, provided for backward compatibility.")
optional.add_argument("--skip-branding", action="store_true", default=False,
help="Disable automatic branding package selection. Use --installpkgs to add custom branding.")
optional.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs",
Expand Down Expand Up @@ -328,8 +340,8 @@ def lmc_parser(dracut_default=""):
parser.add_argument("--releasever", default=DEFAULT_RELEASEVER,
help="substituted for @VERSION@ in bootloader config files")
parser.add_argument("--volid", default=None, help="volume id")
parser.add_argument("--squashfs-only", action="store_const", const="squashfs",
dest="rootfs_type",
parser.add_argument("--squashfs-only", action=NullAction,
help="Ignored, provided for backward compatibility.")
help="Use a plain squashfs filesystem for the runtime.")
parser.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs",
help="Type of rootfs: %s" % ",".join(ROOTFSTYPES))
Expand Down

0 comments on commit 0193ff5

Please sign in to comment.