Skip to content
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

load local dependencies in inspec shell #2438

Merged
merged 5 commits into from
Jan 4, 2018
Merged

load local dependencies in inspec shell #2438

merged 5 commits into from
Jan 4, 2018

Conversation

arlimus
Copy link
Contributor

@arlimus arlimus commented Jan 3, 2018

for loading dependencies from local folders. mainly used for development.

depends

Fixes #1486

for loading dependencies from local folders. mainly used for development.

Signed-off-by: Dominik Richter <[email protected]>
@arlimus arlimus requested a review from a team as a code owner January 3, 2018 02:36
@arlimus arlimus added the Type: Enhancement Improves an existing feature label Jan 3, 2018
Signed-off-by: Dominik Richter <[email protected]>
@arlimus arlimus force-pushed the dom/shell-depends branch 2 times, most recently from 88b24f1 to ed16a92 Compare January 3, 2018 02:47
Signed-off-by: Dominik Richter <[email protected]>
@arlimus arlimus force-pushed the dom/shell-depends branch from ed16a92 to 9f14af3 Compare January 3, 2018 02:47
@aaronlippold
Copy link
Collaborator

That's really cool. Now the follow-up to this would be to add train support for AWS.

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super great change, @arlimus, and closes a long-standing feature request too!

Two suggestions, neither of which are blocking.

@@ -194,6 +194,8 @@ def detect
desc: 'A single command string to run instead of launching the shell'
option :format, type: :string, default: nil, hide: true,
desc: 'Which formatter to use: cli, documentation, html, json, json-min, junit, progress'
option :depends, type: :array, default: [],
desc: 'A list of local folders that are loaded as dependencies (e.g. for resources)'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposed alteration to make it clear what type of folder we're expecting:

'A space-delimited list of local folders containing profiles whose libraries and resources will be loaded into the new shell'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much nicer words, thank you, i'll add it!

@@ -194,6 +194,8 @@ def detect
desc: 'A single command string to run instead of launching the shell'
option :format, type: :string, default: nil, hide: true,
desc: 'Which formatter to use: cli, documentation, html, json, json-min, junit, progress'
option :depends, type: :array, default: [],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about calling the option --load-profiles instead of --depends? I know --depends is technically correct re: implementation and consistent with the inspec.yml, but I wonder if --load-profiles is a clearer option as to what it feels like to the user. Don't feel obligated to take this suggestion, just wanted to toss it out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Load-profiles is interesting too yeah 🤔

To add a small argument: "load profiles" would technically only load the libraries of the profile and not the controls, i.e. not the entire profile. I was pondering "load libraries" (but that may lead users to specify the libraries folder which we don't want either; traversing dependencies for all resources is beautiful right now)

There may be a better term...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--load-profile-libs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of anything terribly better. If --load-profile-libs doesn't feel right, and you think --depends is clear enough, please merge when ready. :) We can always add an alias if someone comes up with something better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, i'll make sure to add the docs you suggested below. Otherwise we can deprecate and find something else. Naming is hard...

@adamleff
Copy link
Contributor

adamleff commented Jan 3, 2018

Could we add a quick section to https://www.inspec.io/docs/reference/shell/ also?

@aaronlippold
Copy link
Collaborator

@arlimus @adamleff do we need to update or add any options to kitchen-inspec as well to connect support for this? Something akin to when controls: and attributes: were added? Looking forward to testing this today or tomorrow 👍

@adamleff
Copy link
Contributor

adamleff commented Jan 3, 2018

The change is shell-specific. It has nothing to do with Test Kitchen or otherwise.

@aaronlippold
Copy link
Collaborator

Ok. Just double checking. Thanks.

@arlimus arlimus force-pushed the dom/shell-depends branch from fe6e981 to 02b498f Compare January 3, 2018 18:19
@arlimus arlimus force-pushed the dom/shell-depends branch from 8ed49fe to 4fe6e66 Compare January 3, 2018 22:20
@adamleff adamleff merged commit be9ece6 into master Jan 4, 2018
@arlimus arlimus deleted the dom/shell-depends branch January 27, 2018 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants