-
Notifications
You must be signed in to change notification settings - Fork 48
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
(HC-31) Handle type change from scalar -> array #26
Conversation
The reason we're seeing this issue is because of all the weirdness around Puppet's In light of that, I'm good with the approach taken here. It feels like the best approach given the circumstances. 👍 |
File.open(tmpfile, 'w') do |fh| | ||
fh.write('ennui: yes') | ||
end | ||
validate_file('ennui: yes') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nitpick, but we probably don't need to validate the file, since it's just testing the behavior of write
, right?
Similar thing in the other test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely. i normally wouldn't do this but i was a stranger in a strange land and any bit of sanity checking was helping me. i left em in because i figured they wouldn't hurt.
I'm also not super familiar with types, providers, and these tests, but this looks good to me. I just had one comment but I'm 👍 |
end | ||
|
||
if type == 'array_element' | ||
Array(@resource[:value]).each do |v| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be return Array(@resource[:value]).any? {|v| value.flatten.include?(v) }
, but I'm not sure if that makes it more readable or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the .any?
makes the intent clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems better. i was just preserving what was there previously.
Previously, if a setting existed on disk as a scalar and the puppet resource changed the setting's type to array, the file would not be updated to reflect this explicit type.
Updated @jpinsonault @haus |
👍 |
1 similar comment
👍 |
(HC-31) Handle type change from scalar -> array
Previously, if a setting existed on disk as a scalar and the puppet resource changed the setting's type to array, the file would not be updated to reflect this explicit type.
In addition to the new unit test, I manually tested this and saw the fix working locally. I am unable to run the acceptance tests.
I've never touched types and providers before and have barely written ruby, so in many ways I feel like I was coding in the dark. Extra scrutiny on this PR would be great!