Skip to content

v2.0: ledger-tool: Fix create-snapshot default value for output_directory (backport of #3148)#3153

Merged
steviez merged 1 commit intov2.0from
mergify/bp/v2.0/pr-3148
Oct 15, 2024
Merged

v2.0: ledger-tool: Fix create-snapshot default value for output_directory (backport of #3148)#3153
steviez merged 1 commit intov2.0from
mergify/bp/v2.0/pr-3148

Conversation

@mergify
Copy link
Copy Markdown

@mergify mergify Bot commented Oct 13, 2024

Problem

The arguments to specify full and incremental snapshot archives paths used to be a global argument; these were moved to only be instantiated on commands that needed them in #1773.

But, when the arguments were moved from app-level to subcommand-level, the code that matches the arguments was not updated to look at subcommand-matches instead of app-matches.

Summary of Changes

Examine the correct matches

Fixes #3117


This is an automatic backport of pull request #3148 done by Mergify.

…3148)

The arguments to specify full and incremental snapshot archives paths
used to be a global argument; these were moved to only be instantiated
on commands that needed them in #1773.

But, when the arguments were moved from app-level to subcommand-level,
the code that matches the arguments was not updated to look at
subcommand-matches instead of app-matches.

(cherry picked from commit 1d9947c)
@mergify mergify Bot requested a review from a team as a code owner October 13, 2024 16:37
@steviez
Copy link
Copy Markdown

steviez commented Oct 13, 2024

My case for BP'ing this is that:

  • There restores behavior that was unintentionally broken
  • The change is very limited, only effects ledger-tool / no downstream deps

@steviez steviez requested a review from brooksprumo October 13, 2024 16:49
Copy link
Copy Markdown

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@t-nelson
Copy link
Copy Markdown

did we varify that this doesn't lose visibility of global args? i vaguely remember fighting this at one point

@steviez
Copy link
Copy Markdown

steviez commented Oct 13, 2024

did we varify that this doesn't lose visibility of global args? i vaguely remember fighting this at one point

Sorry, not sure I follow the question. These two args are now intentionally subcommand args (ie agave-ledger-tool --help will not show them, whereas agave-ledger-tool create-snapshot will), so we need to be matching the subcommand (arg_matches) instead of global (matches). Switching from global to subcommand (as this PR does) for these two args does not affect any other args

@t-nelson
Copy link
Copy Markdown

i have seen arg_matches not contain entries that matches does contain for the same invocation

Copy link
Copy Markdown

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as per zoom call

@steviez steviez merged commit 654cf17 into v2.0 Oct 15, 2024
@steviez steviez deleted the mergify/bp/v2.0/pr-3148 branch October 15, 2024 20:41
@steviez
Copy link
Copy Markdown

steviez commented Oct 15, 2024

Quick context for paper-trail - Trent and I discussed the issues that he had previously seen, and we manually banged around with it. We did not believe there to be any bugs in our code per-se; rather, bugs or abnormalities in our very old version of CLAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants