-
Notifications
You must be signed in to change notification settings - Fork 67
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
Fixes #38006 - Allow extension of list of packages #585
Conversation
adab592
to
d998fdf
Compare
62d1c0e
to
646b8ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @adamruzicka, LGTM.
I've got only two comments:
- Just a note: it kinda seems that there more changes than needed if we only care if any of these packages are installed. If we expect this list to be updated from even other plugins and the name of the package can differ from
scap-security-guide
, then it's fine. Or, iftheme_satellite
must enforce thatscap-sec-g-satellite
must have preference over the default one. - Requirement? :) I've tested with the counterpart, but it failed to load. It seems
BulkUpload
constant is not available duringforeman_theme_satellite
registration inengine.rb
. That is probably I test on Rails 7 though. Long story short, could we movebulk_upload.rb
into eitherapp/lib/foreman_openscap
orapp/services/foreman_openscap
? And just for complicity, remove https://github.com/theforeman/foreman_openscap/blob/master/app/controllers/api/v2/compliance/scap_contents_controller.rb#L1 and https://github.com/theforeman/foreman_openscap/blob/master/test/lib/foreman_openscap/bulk_upload_test.rb#L5 and https://github.com/theforeman/foreman_openscap/blob/master/lib/tasks/foreman_openscap_tasks.rake#L3
I don't really see that happening.
Yeah, it must :/
I'll take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @adamruzicka, and please don't hate me, just a small nitpick. Otherwise, LGTM and seems to be working.
@@ -50,12 +49,12 @@ class BulkUploadTest < ActiveSupport::TestCase | |||
upload = ForemanOpenscap::BulkUpload.new | |||
upload.stubs(:scap_guide_installed?).returns(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though the test passes, we should probably update this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @adamruzicka !
with scap contents