-
Notifications
You must be signed in to change notification settings - Fork 39
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
Refactor to Puppet 4 types #159
Conversation
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.
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.
It looks like I had a draft review that I didn't submit. Could you rebase? I'd like to get this in for 3.5.
manifests/init.pp
Outdated
Boolean $regenerate_ca = $::certs::params::regenerate_ca, | ||
Boolean $deploy = $::certs::params::deploy, | ||
String $ca_common_name = $::certs::params::ca_common_name, | ||
String[2] $country = $::certs::params::country, |
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.
If it needs to be exactly 2 then I think it needs to be String[2,2]
Updated, thanks @chris1984 & @ekohl |
validate_absolute_path($server_cert) | ||
validate_absolute_path($server_cert_req) | ||
validate_absolute_path($server_key) | ||
validate_absolute_path($server_ca_cert) | ||
validate_file_exists($server_cert, $server_cert_req, $server_key, $server_ca_cert) |
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.
Unrelated, but this is the only reason we still need puppet-common. I'm wondering if we can get rid of it somehow.
Tests are broken due to facter. Katello/foreman-installer-modulesync#29 should fix it. |
Since I'm currently in a hotel I don't have access to my testing infrastructure. Anyone can run this through the forklift pipeline and verify it still works? |
Running this with the pipeline fails:
|
@ekohl I'm getting that with & without this PR. I guess it isn't related, as task conflict would be a weird symptom of this change anyway. I haven't had a chance to looks into why its failing yet. |
You're right that's a weird side effect of this patch. You'd either expect a compilation failure due to an incorrect type than a subtle failure somewhere. |
Since we have green nightlies again I was able to retest and it all passed so 👍 |
Thanks @sean797! |
No description provided.