apache: remove support for Apache 2.2 and CentOS 6#9354
Conversation
|
Based on the wording of the issue, looks like this is meant to be merged into the 2.0 branch -- were you thinking of merging it into master for some particular reason or is it ok to change it? |
|
Sorry, yes. All the PRs with 2.0 milestones should be targeting the 2.0 branch. Slipped my mind to set it. |
ohemorange
left a comment
There was a problem hiding this comment.
So, the code here looks good and checks out, other than my one comment. But it might be nice to have someone more familiar with the Apache module give it a pass to see if there are additional places things could be removed from.
| arg_var_interpreter: Pattern = re.compile(r"\$\{[^ \}]*}") | ||
| fnmatch_chars: Set[str] = {"*", "?", "\\", "[", "]"} | ||
|
|
||
| # pylint: disable=unused-argument |
There was a problem hiding this comment.
I mean... if we're doing this for 2.0 anyway, we can just remove version.
There was a problem hiding this comment.
I did try removing version entirely and things got a little bit curly due to how extensively it is used throughout certbot-apache. I can give it another go, but we can also do it another time.
There was a problem hiding this comment.
Oh for reason when looking at this I convinced myself that this was a public signature but we're in _internal here so yeah definitely good with leaving this for now.
bmw
left a comment
There was a problem hiding this comment.
I found a couple small pieces of additional cleanup we could do, but this otherwise LGTM! If you agree with my suggestions, I can also implement them myself Just let me know if you prefer that.
|
|
||
| if self.configurator.version < (2, 4): | ||
| config_template_pre = self.CONFIG_TEMPLATE22_PRE | ||
| config_template_post = self.CONFIG_TEMPLATE22_POST |
There was a problem hiding this comment.
I think we can delete these self.CONFIG_TEMPLATE22* attributes.
| if self.get_version() >= (2, 3, 9): | ||
| rewrite_rule_args = constants.REWRITE_HTTPS_ARGS_WITH_END | ||
| else: | ||
| rewrite_rule_args = constants.REWRITE_HTTPS_ARGS |
There was a problem hiding this comment.
I wonder if we shouldn't do more here with constants.REWRITE_HTTPS_ARGS. It's now only used to detect a redirect rule previously created by Certbot for Apache < 2.3.9. Since that's no longer supported, I wonder if we shouldn't:
- Move the value of
constants.REWRITE_HTTPS_ARGSintoconstants.OLD_REWRITE_HTTPS_ARGS. - Delete
constants.REWRITE_HTTPS_ARGSand all references to it. - We could also rename
constants.REWRITE_HTTPS_ARGS_WITH_ENDtoconstants.REWRITE_HTTPS_ARGSif we wanted, but I'm not sure that's necessary.
I think this has the effect of cleaning up the code a bit and having Certbot update the old redirect rule to the new format if the user is using Apache 2.4. What do you think?
|
Actually, should we add a changelog entry here? How are we going to make sure changes in the 2.0 branch have a changelog entry for Certbot 2.0? |
|
I'm open to ideas. I haven't been writing changelog entries for these PRs in an effort to avoid back-to-back merge conflicts when we put it all together. My vague plan was to do it afterwards. |
|
Works for me! I created #9388 to track that. |
For 2.0.
Fixes #8461.