Define OS options by a dedicated object in Apache configurator#8778
Define OS options by a dedicated object in Apache configurator#8778bmw merged 11 commits intocertbot:masterfrom
Conversation
bmw
left a comment
There was a problem hiding this comment.
Like we talked about in Mattermost, I think we could do more here by moving some of the logic of updating self.options into the _OsOptions class itself, but what you have here is certainly an improvement and maybe moving that logic around is overkill. At the very least, you accomplished what we initially talked about here of giving these attributes well defined types.
Other than my inline comments, this LGTM. I also ran the Apache test farm tests on this PR at https://dev.azure.com/certbot/certbot/_build/results?buildId=3779&view=results and they passed.
| OS_DEFAULTS = dict( | ||
| OS_DEFAULTS = _OsOptions( | ||
| server_root="/etc/apache2", | ||
| vhost_root="/etc/apache2/vhosts.d", |
There was a problem hiding this comment.
Why are we no longer changing vhost_root here?
I think I initially suggested having default values in __init__, but in general I'm personally finding this mix of default and non-default values a little hard to parse. I think watching out for errors like this one is a little tricky and I think it was kind of nice that you could clearly see all of the configuration options in one place while now it's spread across multiple files.
What do you think about removing the default values, requiring all to be set, and setting them explicitly when creating the OS_DEFAULTS object in configurator.py? I think if we did this, we could maybe keep them for values like restart_cmd_alt that only some OSes had, but I also think it'd be fine to just explicitly pass in None in this case.
If we don't do this, this is a reminder to myself to carefully check all of these values for changes like this in all of the override files.
There was a problem hiding this comment.
Well, I am not that convinced it will improve the readability. On top of removing a lot of lines, I appreciate in my proposed approach that the OsDefault class contains the reference configuration that we can after override specifically, which is quite in line with the purpose of the override modules.
I also think that these parts, once properly configured in this PR, will not change much, so we can suppose this will not have a lot of influence in the future.
Also this is not about restart_cmd_alt which is not set, but also ctl or logs_root that are not set in override_arch for instance, so we will end up adding more duplication than before.
However independently from which decision we take here, I am surprised by how options are handled here. If we take again the exemple of override_arch, we see that ctl is not set. Does it mean that the function _prepare_options put None as the first entry for the arrays version_cmd, restart_cmd and conftest_cmd? I suppose this is not the case and a further logic circumvent that, but I fail to understand how this is working. Maybe we should invest some time to improve these handlers in the process?
There was a problem hiding this comment.
NB: I fixed the primary problem here about setting vhost_root for gentoo.
There was a problem hiding this comment.
Well, I am not that convinced it will improve the readability. On top of removing a lot of lines, I appreciate in my proposed approach that the OsDefault class contains the reference configuration that we can after override specifically, which is quite in line with the purpose of the override modules.
I also think that these parts, once properly configured in this PR, will not change much, so we can suppose this will not have a lot of influence in the future.
I still personally disagree as I think the value of OS_DEFAULTS in the base configurator class allows you to see the default values while keeping the definitions contained in one place in one file. In my opinion, the extra lines of code are worth it in this case for simplicity and readability. I'm not going to block this PR on this though so I wrote a simple script to help me verify that no other values were mistakenly changed.
Also this is not about restart_cmd_alt which is not set, but also ctl or logs_root that are not set in override_arch for instance, so we will end up adding more duplication than before.
I think you're mistaken and ctl and logs_root are set in every override class. Here are those values in override_arch.py:
certbot/certbot-apache/certbot_apache/_internal/override_arch.py
Lines 16 to 17 in e33090f
|
@bmw I missed or misinterpreted your comment about adding some logic about updates. But surely this would add some value to this new object. So if you do not mind, I will add it to this current PR. |
If you want to do it, go for it! I think it's a further improvement, I just also think it's OK to stop here if you want since you accomplished what you initially set out to do of giving these objects well defined types. |
|
It looks like there are some merge conflicts, but was there more you wanted to add here? Either way, what do you think about me giving this another review and merging this PR if I'm happy with it? I'd be happy to review any subsequent PRs as well if you wanted to expand things further. |
|
No as of now I am fine. I will make another PRs to include the specific logic on some options params inside the new class. |
# Conflicts: # certbot-apache/certbot_apache/_internal/override_centos.py # certbot-apache/certbot_apache/_internal/override_fedora.py # certbot-apache/certbot_apache/_internal/override_gentoo.py
bmw
left a comment
There was a problem hiding this comment.
The approach taken in the discussion at #8778 (comment) isn't my first choice and I do think we could move preparing options into the OsOptions class which might clean things up a bit more, but this LGTM!
In #8748 (comment) we discussed about changing the dict used to set OS options for Apache configurators into a dedicated object.
This PR does that.