Preserve --must-staple in configuration for renewal (#3844) #3849
Conversation
|
@schoen do you have time to review this? |
| if config_value in ("True", "False"): | ||
| # bool("False") == True | ||
| # pylint: disable=eval-used | ||
| setattr(config.namespace, config_item, eval(config_value)) |
There was a problem hiding this comment.
Any chance of not using the eval so as not to have to disable the pylint warning? I can see why it's safe here, but maybe it would be better not to have to have someone else look into that later on.
We could also say:
config_value = (config_value == True)
or
config_value = True if config_value == "True" else False
Do you think either of those is less clear than the eval() form, also taking into account the two comment lines that you had to add to justify the eval?
|
@thomaszbz, I liked this (and it's exactly parallel to the existing types), except that I questioned the use of Thanks. |
|
@schoen I agree, I did not like this as well. It's done similar to a few lines above, for consistency. To avoid the eval(), I can just return the result of a comparison ("True" or "False" are checked before). |
|
@thomaszbz, I think that would be preferable overall. |
|
@schoen Could you please review this PR again? Someone still must test if the change actually does what it is supposed to (see first comment). It's untested by me so far. |
| @@ -190,6 +191,15 @@ def _restore_required_config_elements(config, renewalparams): | |||
| raise errors.Error( | |||
| "Expected a numeric value for {0}".format(config_item)) | |||
| setattr(config.namespace, config_item, int_value) | |||
| # bool-valued items to add if they're present | |||
There was a problem hiding this comment.
The test coverage of renewal.py is a mess for historical reasons (they're mostly in certbot/tests/main_test.py, and are more like functional tests than unit tests), but we should probably use each PR landing there as an opportunity to migrate towards consistent unit test coverage. So let's try to add a simple unit test for at least for the new BOOL_CONFIG_ITEMS logic, and maybe for all of _restore_required_config_elements to certbot/tests/renewal_test.py as part of this PR.
@thomaszbz, let us know whether you'd be comfortable tackling that, or might need guidance or help in writing unit tests.
There was a problem hiding this comment.
Sorry, I currently can't even test if it runs without a unit test ;-) I gave it a try already (first commit), but I can't do that without learning a lot of python specific stuff. Which I currently don't want to.
Unless it's easy to do... I just don't know yet.
|
@thomaszbz, I'm sorry to say I was on holiday during most of the relevant time so I didn't look at this, and as you might have seen, @bmw ended up duplicating much of this PR (plus some other relevant stuff that we need). We just merged a version of his work on top of this PR and are planning to merge it shortly... sorry for the duplication of effort involved, and thank you for your work on this. |
See #3844: Preserves
--must-staplefor renewal.This patch is completely untested! Review carefully (my first commit to certbot, and my first loc in python).
All I can say is that the testsuite still runs in docker and that I found a few places that somehow qualify to be patched for #3844.
If someone knows how to easily simulate/bypass the whole domain checking process in Docker (with e.g. an untrusted cert, of course), I could eventually test it on my dev system.
--dry-runand--stagingstill did domain checking.