Skip to content

Commit

Permalink
Accept but ignore the old --squashfs-only argument
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 specify a completely no-op argument, so
let's just keep it as `store_true` like it used to be, and
simply never use the value.

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 aafedbb
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 8 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
10 changes: 4 additions & 6 deletions src/pylorax/cmdline.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,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="store_true",
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,9 +327,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",
help="Use a plain squashfs filesystem for the runtime.")
parser.add_argument("--squashfs-only", action="store_true",
help="Ignored, provided for backward compatibility.")
parser.add_argument("--rootfs-type", metavar="ROOTFSTYPE", default="squashfs",
help="Type of rootfs: %s" % ",".join(ROOTFSTYPES))
parser.add_argument("--timeout", default=None, type=int,
Expand Down

0 comments on commit aafedbb

Please sign in to comment.