Replace (most) global state in cli/__init__.py#9678
Conversation
|
Added plenty more tests, this is now review-ready! |
ohemorange
left a comment
There was a problem hiding this comment.
I am so glad to see this, overall looks super clean and allows the reader to reason about its expected actions and trace how values are set and changed.
After arguments/config files/user prompted input have been parsed, we build a mapping of Namespace options to an ArgumentSource value. These generally come from argparse's builtin "source_to_settings" dict, but we also add a source value representing dynamic values set at runtime. This dict is then passed to NamespaceConfig, which can then be queried directly or via the "set_by_user" method, which replaces the global "set_by_cli" and "option_was_set" functions.
This involves passing the NamespaceConfig around to more functions than before, removes the need for most of the global state shenanigans needed by set_by_cli and friends.
This'll correctly mark them as being "runtime" values in the ArgumentSources dict
We need a version that has get_source_to_settings_dict()
One of the tests revealed that ConfigArgParse's source dict excludes arguments it considers unimportant/irrelevant. We now mark all arguments as having a DEFAULT source by default, and update them otherwise.
We were already testing most of these cases in cli_test.py, only with a more complete HelpfulArgumentParser setup. And since the hsts/no-hsts test was manually performing the kind of argument adding that cli already does out of the box, I figured the cli tests were a more natural place for it.
The dict is now built in a defined order: first defaults, then config files, then env vars, then command line args. This way we eliminate the possibility of undefined behavior if configargparse puts an arg's entry in multiple source dicts.
ohemorange
left a comment
There was a problem hiding this comment.
love it! very excited to get this in!
| def __init__(self, namespace: argparse.Namespace, | ||
| argument_sources: Dict[str, ArgumentSource]) -> None: |
There was a problem hiding this comment.
I think this is a breaking change to our public API.
I'm looking around in GitHub code search and I'm seeing a couple of projects that do construct NamespaceConfig, using only one argument to the constructor. I don't know if those projects are actively used or not, but it's probably good to stay on the safe side.
Is there a sensible default value that can work here?
There was a problem hiding this comment.
ahh shoot, thanks for catching this. well, the obvious default would just be to pass an empty dict as a default value, but this means that the set_by_user(...) check will always be false, which may break things in subtle ways for users.
i guess the safest option here would be to revert and wait for a major version release.
There was a problem hiding this comment.
Sooooooo we could maybe move set_by_user somewhere _internal instead of having it be a method on NamespaceConfig. Since set_by_cli was internal anyway and therefore people weren't (shouldn't have been) using it, so they probably don't need to be checking that anyway. Then we could set the default to None and use it more as a backwards compatibility flag and raise an error if we see it when we call set_by_user.
There was a problem hiding this comment.
I actually think this might make sense to do anyway, because I don't think people calling it will want to have to fix their usage of it especially when they're not actually using that part of the functionality, and this will let them continue to use more newer versions of certbot. In that case we could either add a default value and use it as a flag, or just use a different constructor/utility function internally that sets argument_sources after construction.
There was a problem hiding this comment.
so i agree that set_by_user shouldn't be in the public API, the problem is more that whenever we internally call it, a NamespaceConfig with an empty/None argument_sources will still always register all arguments as being not set_by_user. so e.g.
certbot/certbot/certbot/_internal/storage.py
Line 285 in a5d223d
There was a problem hiding this comment.
i guess i feel like proper usage of NamespaceConfig w/ this change will now always require setting up an accurate argument_sources, which is a somewhat tricky ask for public API
There was a problem hiding this comment.
I do see what you're saying -- it's a little weird conceptually to have _mark_runtime_override_ inside the class and _set_by_user outside, though we could for sure make it work using flags/assertions. Because I think it's only true that we really need to set up argument_sources properly if we plan to call set_by_user.
Anyway, another option would be to subclass NamespaceConfig and use that internally, and just add the extra functionality there. Rather than just moving set_by_user to _internal.
There was a problem hiding this comment.
What I'm trying to get at with all this is I think there's benefit to not breaking this interface at all instead of waiting for 3.0 given how it's used externally, and I think we can find a way to do that cleanly, whether it's one of these two things or something else.
There was a problem hiding this comment.
that makes sense! i'll open another PR that reverts the constructor interface & raises exceptions if argument_sources isn't set when set_by_user is called
This reverts commit a5d223d.
I neglected to do this during #9678, so looks like some pip installs are failing to get the minimum required version.
I neglected to do this during #9678, so looks like some pip installs are failing to get the minimum required version.
* Bump setup.py's ConfigArgParse version (#9784) I neglected to do this during #9678, so looks like some pip installs are failing to get the minimum required version. (cherry picked from commit 02efc8c) * Fix dnsimple typo (#9787) Fixes #9786. (cherry picked from commit 4e60a0d) * update pinned dependencies (#9788) This fixes the security alerts those with access can see at https://github.com/certbot/certbot/security/dependabot. (cherry picked from commit 5849ff7) * update changelog for configargparse (#9789) I'd like to do a bug fix release for #9786. If we're doing one, I figure we may as well flag this change from #9784 too. (cherry picked from commit 61773be) --------- Co-authored-by: Will Greenberg <willg@eff.org>
This PR sets up a dict of
ArgumentSources (e.g.COMMAND_LINE,DEFAULT,CONFIG_FILE, etc.) for every field inNamespaceConfig. This way we can do the same checks thatset_by_cliwas doing in a non-globally-mutating way.The global parser is still setup by
cli/__init__.pyto get argument types, and I could try to get rid of that as well in this PR, but from what I could tell our biggest issues were coming from thedetectorsituation.Fixes #9598, fixes #6164.