helpful: fix handling of abbreviated ConfigArgparse arguments

[wgreenberg]

ConfigArgparse allows for "abbreviated" arguments, i.e. just the prefix of an argument, but it doesn't set the argument sources in these cases. This commit checks for those cases and sets the sources appropriately.

Fixes #9793

[bmw]

I'm impressed! I personally expect anything related to any of our argument parsing to be a bit of a can of worms but you figured it out with a patch that makes sense to me very quickly. Nice work.

Two questions:

1. Is just logging when we can't find the corresponding action what we want to do? Are there any alternative approaches you considered? I don't think I fully grok the implications of this.
2. Do you think we should mention this in the changelog? I'm not sure if that was a purposeful omission or just something that wasn't thought about yet.

[wgreenberg]

1. an action should always be found, so it's technically an error state if it isn't. my original thinking was that this error state isn't fatal, but should be logged in case it causes downstream issues, but perhaps it's actually best to error out when it happens? the result of an action not being found would be false negatives from set_by_user(ARG_WITH_NO_ACTION), which looks like it might lead to some confusing behavior
2. d'oh i always forget a good changelog entry (despite it being in the PR template 🤦).

[bmw]

1. Ah I see. I'm honestly on the fence. Normally I personally like the idea of failing fast, but it sounds like it's unlikely to matter much and it also looks like continuing without modifying result was our previous behavior for arguments set on the command line too.
2. Haha no worries. I personally think in cases like this it is borderline anyway. My thinking basically is will someone in the future be trying to figure out in what release this was fixed? At this point, it seems unlikely.

If you want to change anything in response to (1) or (2) feel free and I'll prioritize giving it it another review but also feel free to merge as is.

[wgreenberg]

Maybe a compromise for 1 could be upgrading the log statement to .error(), so we'll at least get some heads up if it happens? Though it's difficult to think of a user-facing way of describing the issue clearly.

[bmw]

Yeah. I think you're right both that it's kind of a compromise and that it's hard to communicate. Mainly for the latter reason, I think I personally prefer our two original options:

1. Blow up to ensure the error doesn't propagate and show up in unexpected ways.
2. Debug logging like this PR currently does to help devs track down the problem if unexpected errors related to this ever happens.

Both seem unlikely though so I'm personally not too worried about it anymore.

[wgreenberg]

Sounds good, I think I'll opt for 1 since some of the false-positives from .set_by_user() look like they'd be both really annoying and subtle to notice. thanks!

[bmw]

Ooo it looks like the tests are triggering the case we (or at least I) thought shouldn't happen. Maybe annoying, but also maybe for the best in helping us understand all this and to do The Right Thing™ here.

One minor structural suggestion that you should feel free to push back on though: If we're going to error, maybe we should error inside _find_action_for_arg instead of returning None? I think it'd simplify things a bit and help keep things consistent.

[wgreenberg]

blowing things up always pays off! and sounds good re: moving the exception, i had two separate cases of it to make the error message slightly more descriptive, but it's probably not necessary

[wgreenberg]

sorry for the mad commit churn on this, but i think it's in a good spot now for re-review. the CI caught some important edge cases which are now addressed!

[bmw]

No problem at all on the commit churn. I was going to wait for you to ping me or ask you before rereviewing.

PR LGTM except I think the changelog entry will get added to the 2.7.1 release when this is merged. Sorry for the inconvenience. You can fix this by merging in master and fixing up the changelog.

We/I should maybe tackle #8272, especially since I learned about towncrier.

[wgreenberg]

changelog fixed! also towncrier looks great, i'll take a look at getting something going w/ it

[bmw]

also towncrier looks great, i'll take a look at getting something going w/ it

There's certainly no pressure from my end for you to do so, but #8272 was a newer version of #6462 so this issue has been an annoyance for Certbot devs for a long time that we've never prioritized fixing for whatever reason and it'd be awesome to see some progress on it.