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

Add additional SUSE and openSUSE platforms - micro, hpc #1753

Merged
merged 4 commits into from
Aug 9, 2022
Merged

Add additional SUSE and openSUSE platforms - micro, hpc #1753

merged 4 commits into from
Aug 9, 2022

Conversation

jamesongithub
Copy link
Contributor

@jamesongithub jamesongithub commented Jun 23, 2022 •
edited
Loading

Description

Add additional platforms.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

@jamesongithub jamesongithub requested review from a team as code owners June 23, 2022 01:19
jamesongithub
jamesongithub commented Jun 23, 2022
View reviewed changes
lib/ohai/plugins/linux/platform.rb Outdated
@@ -149,7 +149,7 @@ def platform_family_from_platform(plat)
"rhel"
when /amazon/
"amazon"
when /suse/, /sles/, /opensuseleap/, /opensuse/, /sled/
when /suse/, /sles/, /opensuseleap/, /opensuse-tumbleweed/, /opensuse-leap-micro/, /opensuse-microos/, /suse-microos/, /opensuse/, /sled/, /sle_hpc/, /sle_rt/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

woudl be nice to be able to remove these 'platform_id_remaps' and add them here instead

opensuse-leap" => "opensuseleap"
sles_sap" => "suse"

looks like this could potentially break if recipes depend on this remapping tho

@jamesongithub
Add additional SUSE and openSUSE platforms - micro, hpc
2b44144 
Signed-off-by: jamesongithub@users.noreply.github.com <jamesongithub@users.noreply.github.com>
@johnmccrae
Copy link
Contributor

@jamesongithub can you please sign your commit too by adding a "signed_by" tag (or git commit -sm "foo")

jaymzh
jaymzh requested changes Jun 28, 2022
View reviewed changes
Copy link
Collaborator

@jaymzh jaymzh left a comment

Choose a reason for hiding this comment

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

These are redundant.

lib/ohai/plugins/linux/platform.rb Outdated
@@ -149,7 +149,7 @@ def platform_family_from_platform(plat)
"rhel"
when /amazon/
"amazon"
when /suse/, /sles/, /opensuseleap/, /opensuse/, /sled/
when /suse/, /sles/, /opensuseleap/, /opensuse-tumbleweed/, /opensuse-leap-micro/, /opensuse-microos/, /suse-microos/, /opensuse/, /sled/, /sle-hpc/, /sle_hpc/, /sle_rt/
Copy link
Collaborator

@jaymzh jaymzh Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

/opensuse/ and /suse/ catch most of these.

Suggested change
when /suse/, /sles/, /opensuseleap/, /opensuse-tumbleweed/, /opensuse-leap-micro/, /opensuse-microos/, /suse-microos/, /opensuse/, /sled/, /sle-hpc/, /sle_hpc/, /sle_rt/
# suse matches opensuse, suse-* opensuse-*, etc.
when /suse/, /sle[sd-_]/

Copy link
Contributor Author

@jamesongithub jamesongithub Jun 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

oh didn't know this was regex, updated. thx

@jamesongithub
Copy link
Contributor Author

i signed it. i guess this just checks the last commit?

@jamesongithub
Copy link
Contributor Author

jamesongithub commented Jun 29, 2022 •
edited
Loading

would it be possible remove this:

ohai/lib/ohai/plugins/linux/platform.rb

Line 122 in a67a053

"sles_sap" => "suse",
?

could break existing recipes, but wondering how many of them rely on this..

as is we can't use platform to distinguish sles and sles_sap

@jaymzh
Copy link
Collaborator

jaymzh commented Jul 19, 2022

i signed it. i guess this just checks the last commit?

it checks all commits. Every line of code requires you to attest you own it and can give it away :)

@jamesongithub
sle-hpc for 12.5
fd9ac13 
Signed-off-by: jamesongithub@users.noreply.github.com <jamesongithub@users.noreply.github.com>
@jamesongithub
use regex
5b49430 
Signed-off-by: jamesongithub@users.noreply.github.com <jamesongithub@users.noreply.github.com>
@jamesongithub
spelling
037302f 
Signed-off-by: jamesongithub@users.noreply.github.com <jamesongithub@users.noreply.github.com>
@sonarcloud
Copy link

sonarcloud bot commented Jul 20, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jamesongithub
Copy link
Contributor Author

kk, think they are all signed now

@jamesongithub
Copy link
Contributor Author

would it be possible remove this:

ohai/lib/ohai/plugins/linux/platform.rb

Line 122 in a67a053

"sles_sap" => "suse",

?
could break existing recipes, but wondering how many of them rely on this..

as is we can't use platform to distinguish sles and sles_sap

thoughts?

@jaymzh jaymzh merged commit c7a4547 into chef:main Aug 9, 2022
@jamesongithub jamesongithub deleted the jy/suse branch August 9, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymzh jaymzh jaymzh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jamesongithub @johnmccrae @jaymzh