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

Document openscap lib required for openscap gem to work #189

Merged
merged 1 commit into from
Mar 3, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Feb 11, 2017

Unusually, since the gem uses FFI at runtime, you don't need this lib
for bundle install to succeed, but require 'openscap' will fail.

Moreover, app/models/openscap_result.rb currently swallows the require failure,
resulting in more confusing undefined method `oscap_cleanup' error:
https://gist.github.com/cben/11af8299c6c85111f5355b6db8c750ce

@moolitayer please review.

  • Is openscap package enough without openscap-devel? Seems to work.
  • This is too easy to miss, I was never aware I need it.
    Could we maybe arrange for bundler or bin/setup to fail / give warning
    without it?
  • I think with_openscap_arf should validate openscap_available?
    returned true, but that's a separate discussion.

@isimluk
Copy link
Member

isimluk commented Feb 12, 2017

Is openscap package enough without openscap-devel?

Yes. It would be bug otherwise.

@@ -26,6 +26,7 @@
rpm -q --whatprovides npm || sudo dnf -y install npm # For CentOS 7, Fedora 23 and older
sudo dnf -y install openssl-devel # For rubygems
sudo dnf -y install cmake # For rugged Gem
sudo dnf -y install openscap # For openscap Gem (the Gem installs without the lib but require fails)

Choose a reason for hiding this comment

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

👍 I would remove what's in the parenthesis as I think it is not interesting for someone setting up a development environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@moolitayer
Copy link

Is openscap package enough without openscap-devel? Seems to work.

Yes you do not need devel as we are only using the openscap binary, not building against it.

This is too easy to miss, I was never aware I need it.
Could we maybe arrange for bundler or bin/setup to fail / give warning
without it?

This was a requirement, when we first introduced openscap core told us that there will be no option to
install binaries on integration servers. The target was to write code (& tests) that would work locally if the gem is installed, but not fail Travis.

Note at appliance and podified the Binary is installed. I added some warning at require failures so that customers that encounter the problem on a new environment that has it's own dependency management (like we recently had with podified ManageIQ) will be able to easily report what is wrong.

Unusually, since the gem uses FFI at runtime, you don't need this lib
for `bundle install` to succeed, but `require 'openscap'` will fail.
Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 👍
(better assign this one to someone active on this file, I don't know if it is done automatically here)

@simon3z
Copy link

simon3z commented Feb 20, 2017

@miq-bot assign isimluk

@isimluk can you review and have this merged when ready?

@simon3z
Copy link

simon3z commented Feb 24, 2017

Ping @isimluk

@simon3z
Copy link

simon3z commented Mar 1, 2017

@miq-bot assign isimluk

Copy link
Member

@isimluk isimluk left a comment

Choose a reason for hiding this comment

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

👍

@isimluk
Copy link
Member

isimluk commented Mar 2, 2017

I am sorry but I can't merge this.

@simon3z
Copy link

simon3z commented Mar 2, 2017

@roliveri can you merge this?

@roliveri
Copy link
Member

roliveri commented Mar 2, 2017

@simon3z Sorry I can't merge this. It seems I don't have merge permission for the guides repo.

@cben
Copy link
Contributor Author

cben commented Mar 2, 2017

@Fryguy @chessbyte please merge

@Fryguy
Copy link
Member

Fryguy commented Mar 3, 2017

cc @carbonin

I'll merge for now, but I really don't understand why this is required. We should not be requiring openscap unless it's needed, and this should be an optional dep.

@Fryguy Fryguy merged commit 22d0674 into ManageIQ:master Mar 3, 2017
@simon3z
Copy link

simon3z commented Mar 3, 2017

I'll merge for now, but I really don't understand why this is required. We should not be requiring openscap unless it's needed, and this should be an optional dep.

@Fryguy yes it should have been mentioned as optional in this doc change.
@cben can you add a comment on the fact that it's optional and needed only to display the openscap results (IIRC).

@cben
Copy link
Contributor Author

cben commented Mar 5, 2017

@Fryguy it's optional as in manageiq will not crash, but some of the container SSA functionality will be skipped. AFAICT it affects not only display but also collection of OpenscapResult (collect_compliance_data) and its parsing into OpenscapRuleResults (create_rule_results).

IMHO developer setup should document how to get a fully functional MIQ. If we the code uses the dependency in prod, developers should also exercise it. FWIW, I've worked on container team for nearly a year until I even realized (by digging deep in log, and stepping through this code in byebug) that there is functionality I'm missing!

Sure, opened #195 to clarify it's optional.

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.

6 participants