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

replace targets with fetcher+reader system #473

Merged
merged 28 commits into from
Feb 22, 2016
Merged

Conversation

arlimus
Copy link
Contributor

@arlimus arlimus commented Feb 22, 2016

Fully removes the current target system, which typically worked this way:

target ---> resolve( ) --->  [ metadata, ruby_tests, ruby_libs ]

Although incredibly convenient, this system was convoluted and hard to understand, as it did 2 things at once: Fetch the target you are interested in and then understand it (as either an InSpec profile or flat Ruby files, though there was some early support for Serverspec folders).

The new system instead works like this:

#1: get raw contents; e.g. file, folder, zip, tar, url, ...
target ---> fetch( ) ---> Fetcher[target]
                          => files(), read(file)

# optional: find the root folder
Fetcher[target] ---> relative_target() --->  Fetcher[target@root_dir]

#2: understand contents; e.g. inspec, flat, scap, ...
Fetcher[target@root_dir] ---> read_source( ) --->  SourceReader[Fetcher[target]]
                                                   => metadata(), tests(), libraries()

What appears to be more complex, now fully exposes all steps and clearly defines them. It provides explicit access to many elements of that chain, that were so far hidden, and led to a lot of code-duplication (e.g. source-paths, especially when chaining fetchers; relative-to-root vs absolute paths; raw contents instead of just converted ruby code).

Mainly targets bugs with InSpec's json and check commands and anything that uses the profile class to work.

@arlimus arlimus added the Type: Bug Feature not working as expected label Feb 22, 2016
@arlimus arlimus force-pushed the dr/-target+fetcher branch 4 times, most recently from cc01348 to b5c31a0 Compare February 22, 2016 04:40
# exits on execution:
runner = Inspec::Runner.new(opts)
profile = Inspec::Profile.for_target(detect_util, opts)
runner.add_profile(profile)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we pass a profile to runners now. We may discuss if we change the direction here as well, but not as part of this PR.

profile = Inspec::Profile.for_target(detect_util, opts)
profile.run

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 point, also a curious one. The advantage of passing multiple things to runner is that it can add all things to a run and then execute them. If we have that execution, would we need Profile.run anymore? I guess it's still great for convenience...

@srenatus
Copy link
Contributor

@chris-rock LGTM.

@arlimus
Copy link
Contributor Author

arlimus commented Feb 22, 2016

Awesome review, thank you @srenatus and @chris-rock !
+1

srenatus added a commit that referenced this pull request Feb 22, 2016
replace targets with fetcher+reader system

discussed with @arlimus, we're merging this although appveyor fails; to fix appveyor asap.
@srenatus srenatus merged commit 376faf1 into master Feb 22, 2016
@srenatus srenatus deleted the dr/-target+fetcher branch February 22, 2016 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants