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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pdksync - (GH-cat-11) Certify Support for Ubuntu 22.04 #2276

Merged
merged 2 commits into from Aug 18, 2022

Conversation

david22swan
Copy link
Member

(GH-cat-11) Certify Support for Ubuntu 22.04
pdk version: 2.4.0

@david22swan david22swan requested a review from a team as a code owner August 4, 2022 10:34
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.

This won't be enough. See #2259 for more.

@david22swan
Copy link
Member Author

Ah, this is just a blanket PR to add to all the metadata's
The actual change's will be added on an on need basis as I go through them all

@ekohl ekohl linked an issue Aug 4, 2022 that may be closed by this pull request
@ekohl
Copy link
Collaborator

ekohl commented Aug 4, 2022

I know, just linking up related issues.

@david22swan david22swan force-pushed the pdksync_GH-cat-11/main/add_ubuntu_22.04_support branch 4 times, most recently from b9e8a6e to 7628202 Compare August 17, 2022 15:05
ekohl
ekohl previously approved these changes Aug 17, 2022
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.

Small stylistic changes, looks good otherwise.

Comment on lines 356 to 357
if ($facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '22.04') < 0) or
($facts['os']['name'] == 'Debian' and versioncmp($facts['os']['release']['major'], '11') < 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indenting looks odd here. Would this work better with the lint plugin?

Suggested change
if ($facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '22.04') < 0) or
($facts['os']['name'] == 'Debian' and versioncmp($facts['os']['release']['major'], '11') < 0) {
if (($facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '22.04') < 0) or
($facts['os']['name'] == 'Debian' and versioncmp($facts['os']['release']['major'], '11') < 0)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Have added the extra circle brackets.
As for the indent while I agree that what you suggested look's better, the syntax check throws a warning at it unfortunately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternative:

  if (
    ($facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '22.04') < 0) or
    ($facts['os']['name'] == 'Debian' and versioncmp($facts['os']['release']['major'], '11') < 0)
  ) {
    # code here
  }

spec/classes/mod/php_spec.rb Show resolved Hide resolved
Comment on lines +356 to +357
if (($facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '22.04') < 0) or
($facts['os']['name'] == 'Debian' and versioncmp($facts['os']['release']['major'], '11') < 0)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I did create os_version_gte for this in the past, but never got around to using it. If you want, you could rewrite the logic here. Be sure to use at least stdlib 8.1.0 then since that fixed the logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

While that function looks like a good idea, doing this would require that a major release be made as we would need to bump the current lower stdlib version bound (it's currently 4.13.1), so I think it may be best to leave as is for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I get you may consider it too large, but normally I don't consider increasing the minimum version a reason for a major version bump. Though 8.1.0 is recentish for stdlib (~1 year old).

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.

We should look at the whitespace lint rule, looks like a bug we should file.

@pmcmaw pmcmaw merged commit 1475cd7 into main Aug 18, 2022
@pmcmaw pmcmaw deleted the pdksync_GH-cat-11/main/add_ubuntu_22.04_support branch August 18, 2022 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP module not working on Ubuntu 22.04 / Jammy
3 participants