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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement JSON schema for inspec exec json outputs #1564

Merged
merged 3 commits into from Mar 22, 2017
Merged

Conversation

arlimus
Copy link
Contributor

@arlimus arlimus commented Mar 14, 2017

This is a current draft to implement JSON schema verification for InSpec json and json-min formatters.

It would also have prevented this #884 (comment) 馃槃

Fixes #884

This is a low-priority item, no rush.

@arlimus arlimus added the Type: Chore How can you have any pudding if you don't eat your meat? label Mar 14, 2017
@arlimus arlimus self-assigned this Mar 14, 2017
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.

This is a huge improvement. Thank you @arlimus. I really like that we have a clear specification now. The only thing that I was confused with is the location of the files in lib/utils/schema. Do we intent do use those in core inspec as well or are they for testing purposes only? If the latter, should we move the directory to test/unit/schema?

'statistics' => statistics,
'version' => { 'type' => 'string' },

# DEPRECATED PROPERTIES!! These will be removed with the next major version bump
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be a good way do communicate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could put it into the output of the generator

@arlimus
Copy link
Contributor Author

arlimus commented Mar 15, 2017

I don't like the location either; lib/utils felt like the least objectionable; I'd prefer not to go into test/*, as this is a verification layer that will be used outside of inspec tests. Traditionally i'd put it into an extra folder somewhere, but didn't want to bloat our file structure yet... (but it might be a better idea)

@chris-rock
Copy link
Contributor

Just discussed with @arlimus offline and we came to the conclusion that the current location is the best one, since we want to use the schema to verify reports too in future.

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.

Left a few comments, and it looks like you have some lint errors to contend with.

@@ -0,0 +1 @@
{"type":"object","additionalProperties":false,"properties":{"profiles":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"name":{"type":"string"},"version":{"type":"string","optional":true},"title":{"type":"string","optional":true},"maintainer":{"type":"string","optional":true},"copyright":{"type":"string","optional":true},"copyright_email":{"type":"string","optional":true},"license":{"type":"string","optional":true},"summary":{"type":"string","optional":true},"supports":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"os-family":{"type":"string","optional":true}}},"optional":true},"controls":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"id":{"type":"string"},"title":{"type":["string","null"]},"desc":{"type":["string","null"]},"impact":{"type":"number"},"refs":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"ref":{"type":"string"},"uri":{"type":"string","optional":true},"url":{"type":"string","optional":true}}}},"tags":{"type":"object"},"code":{"type":"string"},"source_location":{"type":"object","properties":{"ref":{"type":"string"},"line":{"type":"number"}}},"results":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"status":{"type":"string"},"code_desc":{"type":"string"},"run_time":{"type":"number"},"start_time":{"type":"string"},"skip_message":{"type":"string","optional":true},"resource":{"type":"string","optional":true}}}}}}},"groups":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"id":{"type":"string"},"title":{"type":"string","optional":true},"controls":{"type":"array","items":{"type":"string"}}}}},"attributes":{"type":"array"}}}},"statistics":{"type":"object","additionalProperties":false,"properties":{"duration":{"type":"number"}}},"version":{"type":"string"},"controls":"array","other_checks":"array"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any harm in prettyifying this to make it easier for people to see? I realize the output out of the tool isn't "pretty" (and perhaps we should change that) but for purposes of comprehension, it'd be good to see this formatted nicely for humans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We certainly can :) I left it compressed initially to make sure this is just the minified schema. Reasoning should be done via the schema.rb file, changes too; These schemas get complex rather quicly where the schema.rb is easy to understand and follow. By making it more human-readable we give more access to it, but may also invite people to try to change the json file directly.

What do you think?

@@ -0,0 +1 @@
{"type":"object","additionalProperties":false,"properties":{"statistics":{"type":"object","additionalProperties":false,"properties":{"duration":{"type":"number"}}},"version":{"type":"string"},"controls":{"type":"array","items":{"type":"object","additionalProperties":false,"properties":{"id":{"type":"string"},"profile_id":{"type":["string","null"]},"status":{"type":"string"},"code_desc":{"type":"string"},"skip_message":{"type":"string","optional":true},"resource":{"type":"string","optional":true}}}}}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as inspec.exec.full.json - would love to see this structured so humans can grok it.

@@ -0,0 +1,164 @@
#!/usr/bin/env ruby
Copy link
Contributor

Choose a reason for hiding this comment

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

For safety/paranoia purposes, should we wrap these methods in a module? I'd hate for something to accidentally require this and have some methods overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, good point! Also valid since it's still in lib/utils instead of some extras folder, so I fully 馃憤 this argument

@arlimus arlimus force-pushed the dr/schema branch 2 times, most recently from 2837a80 to b981b13 Compare March 16, 2017 15:56
@arlimus
Copy link
Contributor Author

arlimus commented Mar 17, 2017

We had 2 more suggestions to make this part of the inspec CLI. While I hesitate to bloat the CLI calls, Adam also mentioned to make it a hidden CLI command. I like this approach, as it is only needed in m2m cases and should not bother regular users. That way the location of lib/utils is justified and won't cause any trouble either. I'll rewrite this.

@arlimus
Copy link
Contributor Author

arlimus commented Mar 18, 2017

@chris-rock @adamleff updated to inspec schema NAME

@chris-rock
Copy link
Contributor

Thank you @arlimus and @adamleff

@chris-rock chris-rock merged commit 2d9d7aa into master Mar 22, 2017
@chris-rock chris-rock deleted the dr/schema branch March 22, 2017 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Chore How can you have any pudding if you don't eat your meat?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants