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

Fixes #14545 - Correctly parse y.z minor OS version from facts #3836

Closed
wants to merge 1 commit into from

Conversation

tbrisker
Copy link
Member

Some OSes use y.z minor version, for example CentOS uses versions such
as 7.2.1511. Currently, the puppet fact parser will only take the 'y'
part of the version, 2 in this case, which can lead to conflicts if the
OS has been defined manually with the 2.1511 minor version (as the
description will still contain the entire version).

"operatingsystem" => "CentOS",
"lsbdistdescription" => "CentOS Linux release 7.2.1511 (Core) ",
"operatingsystemrelease" => "7.2.1511"
}).operatingsystem

Choose a reason for hiding this comment

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

Indent the right brace the same as the first position after the preceding left parenthesis.

@timogoebel
Copy link
Member

LGTM.

@tbrisker : Have you come up with a solution how not to break media paths for, let's say, Centos? Could we just add a new variable that gets replaces in the url that does not contain the full minor version?

@tbrisker
Copy link
Member Author

@timogoebel It looks like actually this will un-break the media path for CentOS, take a look at: http://mirror.centos.org/centos/

@timogoebel
Copy link
Member

@tbrisker : You're correct. 👍

Some OSes use y.z minor version, for example CentOS uses versions such
as 7.2.1511. Currently, the puppet fact parser will only take the 'y'
part of the version, 2 in this case, which can lead to conflicts if the
OS has been defined manually with the 2.1511 minor version (as the
description will still contain the entire version).
@tbrisker
Copy link
Member Author

Thanks to @dlevene1 for providing the reproducing details, please wait for him to verify this solves the issue for him before merging.

@tbrisker
Copy link
Member Author

Reporter has confirmed via mail that this PR fixed the issue for them.

description: "CentOS Linux 7.2.1511")
assert_valid PuppetFactParser.new({ "operatingsystem" => "CentOS",
"lsbdistdescription" => "CentOS Linux release 7.2.1511 (Core) ",
"operatingsystemrelease" => "7.2.1511"
Copy link
Member

Choose a reason for hiding this comment

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

Not that I care, but here's an example where enabling the cop to enforce one type of hash syntax would've been good instead of allowing 2 different hashing syntaxes to coexist.

Copy link
Member Author

Choose a reason for hiding this comment

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

For me it kind of makes sense to use the newer syntax when passing a hash as function arguments. The second hash I think expects string keys rather then symbols, so I kept it with the old style.

@dLobatog
Copy link
Member

Merged as af6d54a, thanks @tbrisker!

@dLobatog dLobatog closed this Sep 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants