fix: unbreak mix igniter.upgrade --git-ci, and positional args after flags - #397
Merged
Conversation
`extract_positional_args/1` walked argv with `OptionParser.next(argv,
switches: [])`. Without a schema `OptionParser` cannot know an option is a
boolean, so it treats the next token as that option's value:
OptionParser.next(["--yes", "igniter"], switches: [])
#=> {:ok, :yes, "igniter", []}
OptionParser.next(["--yes", "igniter"], switches: [yes: :boolean])
#=> {:ok, :yes, true, ["igniter"]}
Any positional argument written after a boolean flag was therefore swallowed
into the flags and silently lost, so `mix igniter.apply_upgrades -y
igniter:0.8.0:0.8.1` failed with "Must provide one or more values for
positional argument `packages`", while the same arguments in the other order
worked.
Pass the task's own switches and aliases, merged with the global options, so
each flag's arity is known. Also consolidates the three byte-identical copies
of this function into the one in `Igniter.Mix.Task`.
Under `--git-ci` the set of packages to upgrade comes entirely from diffing
`HEAD~1:mix.lock` against the loaded deps, and the dependency fetch that
consumes the package list is skipped. The positional arguments are never read,
so demanding them served only to reject a valid invocation:
$ mix igniter.upgrade --git-ci --yes
Must specify at least one package to upgrade or use --all to upgrade all packages.
That is how the shared ash-project CI job invokes the task, so every dependabot
PR in the ecosystem failed this check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The shared
mix igniter.upgradejob in ash-project's reusable CI workflow hasfailed on every dependabot PR across the ecosystem. Investigating it turned up
two independent bugs in igniter.
1.
--git-cirejects its own documented invocationUnder
--git-ci,Igniter.Upgrades.upgrade/1takes the "before" versions fromgit show HEAD~1:mix.lockand skipsapply_and_fetch_dependencies/2entirely,so the upgrade set comes purely from the lockfile diff. The positional package
list is never read on that path —
deps_to_updateis computed and discarded.Requiring it only rejected a valid invocation.
The guard now only applies when it can actually mean something.
2. A boolean flag swallows the positional argument after it
extract_positional_args/1walked argv withOptionParser.next(argv, switches: []).With no schema,
OptionParsercan't tell a boolean flag from one that takes avalue, so it consumes the following token as that flag's value:
So positional arguments written after a boolean flag were silently dropped:
mix igniter.apply_upgrades igniter:0.8.0:0.8.1 -ymix igniter.apply_upgrades -y igniter:0.8.0:0.8.1Must provide one or more values for positional argument packagesmix igniter.upgrade --git-ci jasonThe task's schema was available at the call site all along; it just wasn't
passed to
next/2. This affects every Igniter task with positional arguments,and aliases were affected the same way (
-y fooatefoo).While fixing it, the three byte-identical copies of
extract_positional_args/1(
Igniter.Mix.Task,Igniter.CopiedTasks,Mix.Tasks.Igniter.Install) areconsolidated into the one in
Igniter.Mix.Task, which now takesOptionParseroptions.
Verification
Three regression tests in
test/igniter/mix/task_test.exs. The two coveringflag-then-positional fail on
main; the third asserts a value-taking flagstill consumes its value, guarding against over-correction.
Full suite,
mix credo --strict,mix dialyzerandmix format --check-formattedpass.
Mix.Tasks.Igniter.Phx.InstallTestfails identically before and after(it asserts a generated-file count that depends on the local
phx_new).End-to-end against a scratch project with igniter as a path dep and a
simulated dependabot lock bump, reproducing the CI conditions:
mix.lockis left untouched, confirming the--git-cipath still doesn'tfetch.
Note on a related latent bug, not fixed here
When a dependabot PR adds a dependency,
dep_changes_in_order/2yields{app, nil, version}, and if that new package ships an upgrade taskIgniter.Upgrades.run/5builds the requirement"> #{nil} and <= #{to}"andcrashes with
Version.InvalidRequirementError: invalid requirement: "> and <= 0.8.3".I hit this while setting up the reproduction and left it alone as out of scope —
happy to file it separately.