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

Add `--help` to CLI plugin activation criteria #3757

Merged
merged 2 commits into from Jan 31, 2019

Conversation

Projects
None yet
3 participants
@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii commented Jan 26, 2019

This ensures that all the following result in the same CLI output:

  • inspec
  • inspec help
  • inspec --help

This closes #3753.

Add `--help` to CLI plugin activation criteria
This ensures that all the following result in the same CLI output:
  - `inspec`
  - `inspec help`
  - `inspec --help`

Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Thanks for the quick response on this, @jerryaldrichiii ; just need to consolidate a duplicated test, and make sure we are testing the right thing. Thanks!

activate_me ||= cli_args.empty?
# If the user invoked `inspec help`, `inspec --help`, or only `inspec`
# then activate all CLI plugins so they can display their usage message.
activate_me ||= ['help', '--help', nil].include?(cli_args.first)

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Contributor

This seems a bit less clear, but I think we can live with it.

@@ -28,4 +28,16 @@
out.exit_status.must_equal 0
end
end

describe 'help' do
it 'outputs the same message when using `help`, `--help`, or no args' do

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Contributor

This test (though missing --help) already exists at

[' help', ''].each do |invocation|

Arguably it should be in inspec_test.rb, as you have it , but it shouldn't be in both places. Could you consolidate them?

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Jan 28, 2019

Author Contributor

Sure can! Great catch.

inspec('').stdout,
]

outputs.uniq.length.must_equal 1

This comment has been minimized.

@clintoncwolfe

clintoncwolfe Jan 28, 2019

Contributor

Ahh, this test says "all three of these should say the same message, but we don't care what the contents of the message are".

In fact, we very much do care - the OP on #3753 reported that init was missing from the output. You can do something like

# These are some subcommands provided by core v2 plugins
to verify that plugins are indeed being loaded.

Consolidate tests and verify output contents
Signed-off-by: Jerry Aldrich <jerryaldrichiii@gmail.com>
@clintoncwolfe
Copy link
Contributor

clintoncwolfe left a comment

Thanks!

@clintoncwolfe clintoncwolfe requested a review from miah Jan 30, 2019

@miah

miah approved these changes Jan 31, 2019

@clintoncwolfe clintoncwolfe merged commit 253f7f8 into master Jan 31, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

@clintoncwolfe clintoncwolfe deleted the ja/fix-plugin-load-on-help branch Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment