Add support for defining custom puppet.conf when generating types #993
Conversation
There was a problem hiding this comment.
The request makes sense. I'm a little unclear on the option handling - and I'm unclear if that's my own misunderstanding of what's currently required (we've had regressions in upstream CRI behavior and so have had to do wonky things in the past - or an issue in the code.
However, it copies the functionality of --puppet-path, which imo is what we want. So, assuming this works correctly for the submitter, I'm good with it.
|
@treydock thanks for submitting this pull request! This pull request has some outstanding merge conflicts, are you still interested in getting this PR merged? |
|
I resolved conflicts |
|
hey @treydock , sorry for all the churn, but would you mind resolving conflicts again and rebasing off master? We recently merged changes that fix breakages we were seeing. Thanks so much! |
|
@mwaggett I've rebased |
There was a problem hiding this comment.
I've got a couple of suggestions, but no blockers. @treydock if you see value in making the suggested changes please do; either way we can merge this in the next couple of days. Thanks for the contribution!
lib/r10k/cli/deploy.rb
Outdated
| @@ -31,6 +31,13 @@ def self.command | |||
| exit 1 | |||
| end | |||
| end | |||
| option nil, :'puppet-conf', 'Path to puppet.conf', argument: :required do |value, cmd| | |||
| unless File.readable? value | |||
| $stderr.puts "The specified puppet.conf #{value} could not be read." | |||
There was a problem hiding this comment.
We can use the logger method as it's defined on commands; the following should work:
| $stderr.puts "The specified puppet.conf #{value} could not be read." | |
| logger.fatal "The specified puppet.conf #{value} could not be read." |
(We should also apply the same change for puppet-path, but that's best tracked in another PR.)
lib/r10k/cli/deploy.rb
Outdated
| @@ -31,6 +31,13 @@ def self.command | |||
| exit 1 | |||
| end | |||
| end | |||
| option nil, :'puppet-conf', 'Path to puppet.conf', argument: :required do |value, cmd| | |||
| unless File.readable? value | |||
There was a problem hiding this comment.
We have validation also defined on the setting itself, so this isn't strictly necessary. If we drop this validation, we get the following:
r10k deploy -c r10k.yaml --puppet-conf not_a_file environment
ERROR -> Validation failed for 'global' settings group:
deploy:
Validation failed for 'deploy' settings group:
puppet_conf:
The specified puppet.conf not_a_file is not readable
Is the above error sufficiently clear or are we better served by this more specific error messaging?
There was a problem hiding this comment.
I'm not sure how to make the error more specific. The check is if the file is readable and the error states it's not readable. Not much else to check or tell the user about.
There was a problem hiding this comment.
@treydock I think what @adrienthebo is saying is that we can drop this readable? check altogether because there is validation elsewhere that will produce a similar error message.
There was a problem hiding this comment.
I removed the validation from this particular location.
|
@treydock Would you mind adding an entry to the CHANGELOG for this? Also looks like we need another rebase/merge-conflict-resolution. 😓 |
|
@mwaggett I added a changelog entry and rebased against master. |
mwaggett
force-pushed
the
puppet_conf
branch
from
85b87e1 to
3a92285
Compare
|
This was already approved by @dhollinger but the review got dismissed when I force-pushed to rebase and resolve conflicts in the changelog. so I'm merging even though there is only one recorded review. |
No description provided.