Add proxy settings for r10k #473
Conversation
F5030161 updates module See merge request idigitech/puppet-r10k!1
| @@ -9,6 +9,7 @@ | |||
| $install_options = [] | |||
| $sources = undef | |||
| $puppet_master = true | |||
| $proxy = '' | |||
There was a problem hiding this comment.
I think undef would be a better default.
There was a problem hiding this comment.
I see that both undef and an empty param e.g [] or '' are used throughout the code base, I like setting an empty string because it indicates what type of value is expected and when writing tests we can specifically check for an empty string instead of an undef value. I'm easy either way though. . .. .
There was a problem hiding this comment.
Empty hashes and arrays make more sense but I'd say not for strings.
There was a problem hiding this comment.
to quote our review docs:
Are there new params with datatype Hash or Array? If possible, they should default to empty Hash/Array instead of undef. You can also enforce the datastructure like Array[String]
For simple datatypes like String, please default to undef.
| @@ -22,6 +22,7 @@ | |||
| Hash $deploy_settings = $r10k::params::deploy_settings, | |||
| $root_user = $r10k::params::root_user, | |||
| $root_group = $r10k::params::root_group, | |||
| $proxy = $r10k::params::proxy, | |||
There was a problem hiding this comment.
New parameters should have data types.
| @@ -62,6 +65,7 @@ | |||
| $root_user = $r10k::params::root_user, | |||
| $root_group = $r10k::params::root_group, | |||
| Stdlib::Absolutepath $puppetconf_path = $r10k::params::puppetconf_path, | |||
| String $proxy = $r10k::params::proxy, | |||
There was a problem hiding this comment.
Please also enforce the minimal length:
| String $proxy = $r10k::params::proxy, | |
| Optional[String[1]] $proxy = $r10k::params::proxy, |
| @@ -33,3 +33,6 @@ | |||
| <% if !@forge_settings.empty? -%> | |||
| <%= settings_to_yaml('forge', @forge_settings) %> | |||
| <% end -%> | |||
| <% if @proxy and @proxy != '' -%> | |||
There was a problem hiding this comment.
this can now be simplyfied:
| <% if @proxy and @proxy != '' -%> | |
| <% if @proxy -%> |
|
Hey @Rudikza, thanks for the PR. Can you please take a look at the used email address in the commit? It isn't associated with your github account. |
Pull Request (PR) description
This PR adds in the needed code to be able to manage the proxy setting in the r10k.yaml file.
This Pull Request (PR) fixes the following issues
Fixes #468