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.

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

Already on GitHub? Sign in to your account

Drop EoL Ubuntu code #2481

Merged
merged 1 commit into from Oct 11, 2023
Merged

Drop EoL Ubuntu code #2481

merged 1 commit into from Oct 11, 2023

Conversation

bastelfreak
Copy link
Collaborator

This deletes Ubuntu 16 and older code. Those versions are already gone from metadata.json.

Summary

Provide a detailed description of all the changes present in this pull request.

Additional Context

Add any additional context about the problem here.

  • Root cause and the steps to reproduce. (If applicable)
  • Thought process behind the implementation.

Related Issues (if any)

Mention any related issues or pull requests.

Checklist

  • 馃煝 Spec tests.
  • 馃煝 Acceptance tests.
  • Manually verified. (For example puppet apply)

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

These conditions can be simplified:

spec/classes/mod/php_spec.rb
293:      next if (facts[:os]['release']['major'].to_i > 15 && facts[:os]['name'] == 'Ubuntu') ||

spec/defines/vhost_spec.rb
596:          if os_facts[:os]['release']['major'].to_i >= 18 && os_facts[:os]['name'] == 'Ubuntu'

This deletes Ubuntu 16 and older code. Those versions are already gone
from metadata.json.
@@ -290,9 +290,8 @@
end

# all the following tests are for legacy php/apache versions. They don't work on modern ubuntu and redhat 8
next if (facts[:os]['release']['major'].to_i > 15 && facts[:os]['name'] == 'Ubuntu') ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be simplified to still test for Ubuntu, just like Debian.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests were skipped for those and I updated the code below to match the Debian OS family. The logic should be the same now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yes, I didn't read it carefully enough. Feels like we can also apply the same tricky to the Red Hat OS family below, but that can be done in another PR. Overall I think using next to skip tests below something is a poor practice. Indenting is much more reliable IMHO.

@bastelfreak bastelfreak merged commit b3f5f8c into puppetlabs:main Oct 11, 2023
38 checks passed
@bastelfreak bastelfreak deleted the eol branch October 11, 2023 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants