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

Platform framework and detect DSL #209

Merged
merged 2 commits into from
Nov 15, 2017
Merged

Platform framework and detect DSL #209

merged 2 commits into from
Nov 15, 2017

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Nov 13, 2017

This change moves the detect logic for platforms into a new framework. You can create platforms on the fly using the new platform DSL.

Signed-off-by: Jared Quick [email protected]

@jquick jquick requested review from arlimus, adamleff and a team November 13, 2017 17:58
@jquick jquick force-pushed the jq/platform_objects branch 2 times, most recently from c0303a5 to f9319d5 Compare November 13, 2017 22:02
@jquick jquick force-pushed the jq/platform_objects branch from f9319d5 to 02441ef Compare November 13, 2017 22:08
Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

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

Fantastic huge addition @jquick thank you so much for implementing this crucial part of the platforms RFC!! This is a massive change and should leave most pieces intact while adding a lot of new functionality. 😁

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

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

Awesome work @jquick

@@ -77,8 +74,20 @@ def mock_os(value = {})
# if a user passes a nil value, set to an empty hash so the merge still succeeds
value ||= {}

os_params = { name: 'unknown', family: 'unknown', release: 'unknown', arch: 'unknown' }.merge(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @jquick! We need to ensure mock_os still allows other callers to set the release and arch, and we also need to make sure they're not nil. Can you check to see if you account for this, and if not, could you push up a commit that adds that functionality back?

This will potentially break only_if evaluation during inspec check calls as well as some unit tests potentially (though I think everything in InSpec's unit tests are focused on name and family only).

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.

@jquick this is solid work. I have one concern that I left a comment about - lemme know if it's a non-issue or, if it is, when you've got a fix and then I think this is merge-ready.

@adamleff adamleff merged commit 3ed2adb into master Nov 15, 2017
@adamleff adamleff deleted the jq/platform_objects branch November 15, 2017 20:27
@jquick jquick mentioned this pull request Dec 4, 2017
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants