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

Add correct supports platform to resources. #2674

Merged
merged 3 commits into from
Feb 19, 2018
Merged

Add correct supports platform to resources. #2674

merged 3 commits into from
Feb 19, 2018

Conversation

miah
Copy link
Contributor

@miah miah commented Feb 16, 2018

The PR updates all the resources to specify their supported platforms.

Fixes #2661

Signed-off-by: Miah Johnson <miah@chia-pet.org>
@miah miah requested a review from a team as a code owner February 16, 2018 21:33
Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @miah , This is looking really nice!


require 'utils/simpleconfig'
require 'utils/find_files'

module Inspec::Resources
class ApacheConf < Inspec.resource(1)
name 'apache_conf'
supports platform: 'unix'
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we already have os_family here defined lets update it to be:

supports platform: 'linux'
supports platform: 'debian'

instead of unix, and remove the os_family's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright

@@ -1,6 +1,7 @@
module Inspec::Resources
class FileSystemResource < Inspec.resource(1)
name 'filesystem'
supports platform: 'unix'
Copy link
Contributor

Choose a reason for hiding this comment

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

same here change this to linux and remove the os_family one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it

Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

I think this is a solid platform to start with supports. We can move more restricted later but this will keep them from erroring when used with api's.

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.

I think it is okay to add supports for unix + windows for now, but we need to follow up and add the real support to the resource and remove the checks from initialize.


# Parses a csv document
# This implementation was inspired by a blog post
# @see http://technicalpickles.com/posts/parsing-csv-with-ruby
module Inspec::Resources
class CsvConfig < JsonConfig
name 'csv'
supports platform: 'unix'
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to be careful, since unix and windows is not covering all operating systems. Even we have esx and cisco not being part of unix right now. While thinking about it, we may should fix that in our train config...

Copy link
Contributor

@jquick jquick Feb 19, 2018

Choose a reason for hiding this comment

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

For now lets add:
platform: 'esx'
platform: 'cisco'

To csv, json, and yaml. Once the OS changes go out here inspec/train#261 we can change them to 'os'. But I don't want to force out a detect change this close to 2.0 release.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added this to the PR.

@@ -24,6 +22,7 @@
module Inspec::Resources
class IpTables < Inspec.resource(1)
name 'iptables'
supports platform: 'unix'
Copy link
Contributor

Choose a reason for hiding this comment

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

This would need to be linux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -1,11 +1,11 @@
# encoding: utf-8
# author: Jen Burns, burnsjennifere@gmail.com
Copy link
Contributor

Choose a reason for hiding this comment

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

Have we agreed somewhere that we remove the author headers?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, github accurately keeps history of authors. This is not the main focus of this PR but is being included for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets try to keep a PR focused on one thin in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem, our Team Working Agreement states that we should remove these when the file is touched so thats what I did.

Signed-off-by: Miah Johnson <miah@chia-pet.org>
Signed-off-by: Jared Quick <jquick@chef.io>
@jquick
Copy link
Contributor

jquick commented Feb 19, 2018

@chris-rock - I think this is in a pretty safe spot now with the new changes.

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.

@miah @jquick This is good for a first pass. As a next iteration, we need to focus on:

  • update train to reflect os
  • clean up the supports logic for resources and move that out of the initialize method
  • some resources do not work on unix but only on linux, we need to be more accurate. It works right now, since we have the supports logic still in initialize

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