(PUP-4776) Make 'puppet agent arg' error instead of silently running the catalog #4043
Conversation
|
CLA signed by all contributors. |
|
@peculater thank you for your contribution! The rdoc appveyor failures are unrelated and can be ignored. We'll take a look at this before next week's PR triage on June 30th. |
| @@ -359,6 +359,12 @@ def main(daemon) | |||
| daemon.start | |||
| end | |||
|
|
|||
| def parse_args(args) | |||
There was a problem hiding this comment.
I'm not a fan of having a parse_args method that doesn't really parse the arguments, but instead ensures they are empty.
I think this really could simply be a one liner at the start of setup:
raise ArgumentError, "The puppet agent command does not take parameters" unless command_line.args.empty?
|
@kylog is this considered a breaking change? Before |
|
IMHO this is not a breaking change in the semver sense, i.e. there isn't an established contract that we are changing here. This is just a good old-fashioned bug fix :) |
|
Hi @peculater. I just wanted to check-in to see if you saw my feedback above. Generally this looks good and we should be able to merge this with the recommended changes applied. Thanks! |
|
Hi @peculater. Thanks for pushing up the changes. One last thing and I'll get this merged this afternoon: could you either amend the second commit message to be prefixed with |
…the catalog Currently, 'puppet agent' only analyses options beginning with '--', such as '--test' and '--disable'. Any other words after agent are ignored. That is different from the way that commands like 'puppet resource' and 'puppet apply' work, mainly because those require an argument, while 'puppet agent' does not. This has led to, for instance, one of our new sysadmins typing 'puppet agent disable' instead of 'puppet agent --disable' and being terribly surprised when about 5 minutes later the server died. This patch raises an error if someone on the CLI tries to invoke 'puppet agent' with what has to be an invalid argument.
peculater
force-pushed
the
puppet-agent-cli-should-error-instead-of-daemonize
branch
from
b21e1e2
to
e959744
Compare
|
OK, I think that squashed correctly. :-) |
|
👍 will merge after CI passes. |
…-instead-of-daemonize (PUP-4776) Make 'puppet agent arg' error instead of silently running the catalog
|
@peculater Thanks again for the contribution! This has been merged into the upcoming Puppet 4.3.0 release. |
Currently, 'puppet agent' only analyses options beginning with '--', such as '--test' and
'--disable'. Any other words after agent are ignored. That is different from the way that
commands like 'puppet resource' and 'puppet apply' work, mainly because those require
an argument, while 'puppet agent' does not. This has led to, for instance, one of our new
sysadmins typing 'puppet agent disable' instead of 'puppet agent --disable' and being
terribly surprised when about 5 minutes later the server died.
This patch raises an error if someone on the CLI tries to invoke 'puppet agent' with what
has to be an invalid argument.