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

Fix travis #324

Merged
merged 5 commits into from
Aug 22, 2016
Merged

Fix travis #324

merged 5 commits into from
Aug 22, 2016

Conversation

ghoneycutt
Copy link
Member

No description provided.

The new version handles checking the validity of metadata.json. With
this we no longer need the `rake metadata_lint` as part of the CI
process.
@@ -18,7 +18,7 @@
$python = $::python::version ? {
'system' => 'python',
'pypy' => 'pypy',
default => "${python::version}",
default => "${python::version}", # lint:ignore:only_variable_string
Copy link

Choose a reason for hiding this comment

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

This should be converted to just be default => $python::version rather than disabling the line message. Placing the variable in quotes like this does not cast it to a string, and the behavior both ways is identical. You would need to use sprintf to cast it to a string, but that would actually change the current behavior.

Copy link

Choose a reason for hiding this comment

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

root@testbox:~# cat test.pp 
$version=123
warning(type($version))
warning(type("${version}"))
root@testbox:~# puppet apply test.pp 
Warning: Scope(Class[main]): integer
Warning: Scope(Class[main]): integer

Copy link

@cryptk cryptk Aug 22, 2016

Choose a reason for hiding this comment

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

And doing it another way just to demonstrate the "universality" of how puppet likes to put things into their little boxes:

root@testbox:~# cat test.pp 
$version_int=123
$version_quoted = "${version_int}"
warning(type($version_int))
warning(type($version_quoted))
root@testbox:~# puppet apply test.pp 
Warning: Scope(Class[main]): integer
Warning: Scope(Class[main]): integer

In order to actually cast it into a string, you would need to do something weird like this:

root@testbox:~# cat test2.pp 
$version=123
$version_str = sprintf('"%s"', $version)
warning(type($version_str))
root@cryptkcoding:~# puppet apply test2.pp 
Warning: Scope(Class[main]): string

But again, doing that with sprintf would actually change the current behavior of the module as the current behavior is to treat the $python::version variable as either an int or a str depending on it's contents. Removing the quotes and curly braces will resolve the lint issue while preserving that behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cryptk My only concern is that $python is stringified on purpose and by changing the current behavior it will break something that is expecting that $python is a string.

I'm in favor of doing it your way, though that needs to be tested. Please send a PR that changes the behavior.

Copy link

Choose a reason for hiding this comment

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

I think you misunderstood what I am saying. Placing the variable in quotes like that does NOT stringify it. It is still an integer (assuming that the input is just digits) or a string (if the input is not just digits) either way.

Changing it to read

default => $python::version

is behaviorally identical to the quoted and braced version, just more stylistically correct. Please read my previous line comments which demonstrate this.

@ghoneycutt ghoneycutt merged commit 27c7dc1 into voxpupuli:master Aug 22, 2016
@ghoneycutt ghoneycutt deleted the fix_travis branch August 22, 2016 20:51
@ghoneycutt
Copy link
Member Author

Released in 1.14.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants