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 omnibus for inspec #658

Merged
merged 2 commits into from
Aug 8, 2016
Merged

Add omnibus for inspec #658

merged 2 commits into from
Aug 8, 2016

Conversation

chris-rock
Copy link
Contributor

This is the first step to build packages for InSpec. The current PR includes all files required to build deb and rpm packages.

@chris-rock chris-rock added the Type: Enhancement Improves an existing feature label Apr 18, 2016
@srenatus
Copy link
Contributor

Inspec is already part of omnibus-software, did you not use that for a specific reason?

@chris-rock
Copy link
Contributor Author

@srenatus No, that was not left out intentionally. I just figured out that omnibus software uses bundler, which is not required at all. So I harmonize both, so that we can depend on inspec defined in omnibus-software

name 'inspec'

dependency 'ruby'
dependency 'train'
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 need to define train, since it is defined as gem dependency

@chris-rock chris-rock changed the title add omnibus for inspec WIP: add omnibus for inspec Apr 18, 2016
@chris-rock chris-rock added this to the 0.30.0 milestone Aug 8, 2016
@@ -0,0 +1,3 @@
<% config_files.each do |file| -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the other deb and rpm resource files appear identical to the default versions that omnibus will use. Perhaps we can remove them from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Go ahead, makes sense from my perspective.


# Dependecy added to avoid this pry error:
# "Sorry, you can't use Pry without Readline or a compatible library."
gem "install rb-readline --no-document", env: env
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is required, should we put it in our gemspec instead?

@stevendanna stevendanna self-assigned this Aug 8, 2016
@stevendanna stevendanna changed the title WIP: add omnibus for inspec Add omnibus for inspec Aug 8, 2016
@stevendanna
Copy link
Contributor

stevendanna commented Aug 8, 2016

Ready for Review. Two questions possible changes we could consider:

  • Right now this appbundle's inspec. We've prefered appbundling for other ruby applications because (1) it can help with load times, (2) it helps ensure that we are running with the dependencies we expect even if someone comes along an messes with our gemset. However, this brings in bundler which adds about 4MB to the build. For more info about appbundler see: https://github.com/chef/appbundler
  • We may want to check in omnibus/Gemfile.lock eventually. Because of dependency list is small I've left it unlocked since we aren't likely to see a lot of upstream churn that breaks us.

chris-rock and others added 2 commits August 8, 2016 13:34
- Removes resource files which matched the default implementations
  contained in omnibus.

- Removes software definition for train which will be installed via the
  gem dependecies in the inspec defintion.

- Appbundle inspec to match our other ruby-based projects

- Update rubocop style violations

- Update copyright notices

Signed-off-by: Steven Danna <steve@chef.io>
@chris-rock
Copy link
Contributor Author

@stevendanna Awesome, works as expected!

@chris-rock
Copy link
Contributor Author

I think we should go with appbundler for now and leave the Gemfile.lock out of this PR

@stevendanna stevendanna merged commit 776adc1 into master Aug 8, 2016
@ksubrama
Copy link

ksubrama commented Aug 8, 2016

If you're using appbundler, then you need bundler (to generate the Gemfile.lock), negating the need to write the custom inspec software definition.


# Dependecy added to avoid this pry error: "Sorry, you can't use Pry
# without Readline or a compatible library."
gem 'install rb-readline --no-document', env: env
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ksubrama Can you open a PR to add this improvement?

Copy link

Choose a reason for hiding this comment

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

Can do - I'm noting down a few other issues I noticed and will open a PR will all of the fixes.

@chris-rock chris-rock deleted the chris-rock/omnibus branch August 8, 2016 14:14

# Defaults to C:/inspec on Windows
# and /opt/inspec on all other platforms
install_dir "#{default_root}/#{name}"
Copy link

Choose a reason for hiding this comment

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

Can we prefer C:/opscode/inspec? It is where we land all our other projects and yet another top level directory at root level is going to be a bit annoying. Also, top level directories in a drive has special pitfalls when it comes to permission/inheritance.

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.

None yet

5 participants