rename environment parameter to unbreak hiera #64
Conversation
|
@igalic Please resolve the merge conflicts. |
|
done |
| @@ -9,7 +9,7 @@ | |||
| # precedence over an 'email' setting defined in $config. | |||
| # [*path*] | |||
| # The path to the letsencrypt installation. | |||
| # [*environment*] | |||
| # [*venv_vars*] | |||
There was a problem hiding this comment.
Would you like to add documentation for venv_path at the same time?
|
This is a backwards incompatible change? |
|
Label is the right thing |
|
the basic question is: do we need a release before we merging this, yes/no? |
|
According to SemVer, we should publish a new release, right? |
|
This needs a rebase now due to conflicts. |
This is a fix for #63. We rename the `environment` to `venv_vars` in order to ensure that hiera calls do *not* break, as soon as a puppet execution flow enters our module. For consistency's sake, we change `environment` not only in the main class (where it's definitely needed: rodjek/puppet-lint#574), but also in the `certonly` define.
gosh it's bee a while but it's done |
| @@ -54,8 +54,8 @@ | |||
| class letsencrypt ( | |||
| Optional[String] $email = undef, | |||
| String $path = $letsencrypt::params::path, | |||
| $venv_path = $letsencrypt::params::venv_path, | |||
| Array $environment = [], | |||
| $venv_path = [], | |||
There was a problem hiding this comment.
This doesn't look like a correct rebase
There was a problem hiding this comment.
but now that i'm seeing this, we've already had $venv_vars — is this even still needed?
There was a problem hiding this comment.
the answer is: potentially, but it needs to be rethought, rather than rebased.
There was a problem hiding this comment.
No, we had venv_path so it's still useful to have I think
| @@ -54,8 +54,8 @@ | |||
| class letsencrypt ( | |||
| Optional[String] $email = undef, | |||
| String $path = $letsencrypt::params::path, | |||
| $venv_path = $letsencrypt::params::venv_path, | |||
| Array $environment = [], | |||
| $venv_path = [], | |||
There was a problem hiding this comment.
but now that i'm seeing this, we've already had $venv_vars — is this even still needed?
| @@ -54,8 +54,8 @@ | |||
| class letsencrypt ( | |||
| Optional[String] $email = undef, | |||
| String $path = $letsencrypt::params::path, | |||
| $venv_path = $letsencrypt::params::venv_path, | |||
| Array $environment = [], | |||
| $venv_path = [], | |||
There was a problem hiding this comment.
the answer is: potentially, but it needs to be rethought, rather than rebased.
|
Dear @igalic, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
|
dear @pccibot, as mentioned in my previous communique, this pull request is either obsolete, or it needs to be rethought. Best regards, i |
|
Dear @igalic, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
|
Dear @igalic, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
Fixes #63.
We rename the
environmenttovenv_varsin order to ensure that hiera calls do not break, as soon as a puppet execution flow enters our module. For consistency's sake, we changeenvironmentnot only in the main class (where it's definitely needed: rodjek/puppet-lint#574), but also in thecertonlydefine.