Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop Apache 2.0 compatibility code #2226

Merged
merged 1 commit into from
Apr 5, 2022
Merged

Drop Apache 2.0 compatibility code #2226

merged 1 commit into from
Apr 5, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Mar 29, 2022

It's safe to assume the version is >= 2.2 so the conditional can be dropped.

@ekohl ekohl requested a review from a team as a code owner March 29, 2022 13:30
@puppet-community-rangefinder
Copy link

apache::mod::proxy_connect is a class

Breaking changes to this file WILL impact these 1 modules (exact match):

This module is declared in 174 of 579 indexed public Puppetfiles.

These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

bastelfreak
bastelfreak previously approved these changes Mar 29, 2022
View reviewed changes
ekohl
ekohl commented Mar 30, 2022
View reviewed changes
manifests/mod/proxy_connect.pp Outdated
@@ -2,17 +2,14 @@
# Installs `mod_proxy_connect`.
#
# @param apache_version
# Used to verify that the Apache version you have requested is compatible with the module.
# Deprecated and unused. Remains here for compatibility.
#
# @see https://httpd.apache.org/docs/current/mod/mod_proxy_connect.html for additional documentation.
#
class apache::mod::proxy_connect (
$apache_version = undef,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept this variable in to not make it a breaking change. Silently ignoring it is safe, but it should be removed with the next major release.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we open a PR that drops it? I'm quite sure otherwise we will forget it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about the same thing.

bastelfreak
bastelfreak previously approved these changes Mar 30, 2022
View reviewed changes
smortex
smortex previously approved these changes Apr 5, 2022
View reviewed changes
@chelnak
Copy link
Contributor

chelnak commented Apr 5, 2022

@david22swan This looks good to go to me but I'm wondering if we should include the breaking change mentioned by @ekohl above. There is no real reason we can't do a major release.

What do you think?

@david22swan
Copy link
Member

@ekohl @chelnak Seeing as we have a breaking change already waiting for a release with the removal of OS compatibility code I'd say go for it and remove it.

@chelnak
Copy link
Contributor

chelnak commented Apr 5, 2022

I'm down with that.

@ekohl Feel free to remove the variable in this PR 😄

@ekohl
Drop Apache 2.0 compatibility code
a88bd78 
It's safe to assume the version is >= 2.2 so the conditional can be
dropped.
ekohl
ekohl commented Apr 5, 2022
View reviewed changes
manifests/mod/proxy_connect.pp
@@ -2,17 +2,14 @@
# Installs `mod_proxy_connect`.
#
# @param apache_version
# Used to verify that the Apache version you have requested is compatible with the module.
# Deprecated and unused. Remains here for compatibility.
#
# @see https://httpd.apache.org/docs/current/mod/mod_proxy_connect.html for additional documentation.
#
class apache::mod::proxy_connect (
$apache_version = undef,
) {
include apache
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering if this is still needed, but I wasn't sure. Decided to keep it in since that's safe.

@ekohl ekohl dismissed stale reviews from smortex and bastelfreak via a88bd78 April 5, 2022 10:04
@ekohl
Copy link
Collaborator Author

ekohl commented Apr 5, 2022

Updated to drop the parameter.

@david22swan david22swan merged commit 2dc2bac into puppetlabs:main Apr 5, 2022
@david22swan
Copy link
Member

@ekohl Thanks again for the work :)

@ekohl ekohl deleted the drop-apache-2.0-code branch April 5, 2022 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david22swan david22swan david22swan approved these changes

@bastelfreak bastelfreak bastelfreak approved these changes

@smortex smortex smortex left review comments

Assignees
No one assigned
Labels
backwards-incompatible community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ekohl @chelnak @david22swan @smortex @bastelfreak