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

Adding scan action with userid to container_image #17264

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

nimrodshn
Copy link
Contributor

@nimrodshn nimrodshn commented Apr 9, 2018

What this does?

Currently a ContainerImage scan that was scheduled is not associated with the current userid. This patch allows the user to schedule a ContainerImage scan associated with the current userid.

Why?

Currently scheduled ContainerImage scans are failing due to lack of associated userid required to create the Job.

cc: @cben @agrare @oourfali
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1559459

@nimrodshn
Copy link
Contributor Author

@miq-bot add_label gaprindashvili/yes, bug

@@ -192,7 +192,11 @@ def action_vm_scan(obj, _at)

def action_scan(obj, _at)
sched_action[:options] ||= {}
obj.scan
if obj.respond_to?(:scan_with_userid)
Copy link
Contributor

@lpichler lpichler Apr 9, 2018

Choose a reason for hiding this comment

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

does it make sense to encapsulate this If branch to the scan method ? ( and thanks to that have here only calling scan method )

Copy link
Member

Choose a reason for hiding this comment

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

What other objects is this called on? Can we just always pass the userid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@agrare EmsCluster also calls scan without userid, and without this if I would break their scan (to be honest I'm not acquainted with this entity at all.)

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest we added the new method name and this if because we were unsure what other types other than ContainerImage support schedulable scan action, and hoped one of the reviewers could tell us ;-)
"the best way to get the right answer on the internet is not to ask a question; it's to post the wrong answer."

OK, let's see. scan methods and calls: https://gist.github.com/cben/c582b51e8eb14528fa4ed81243b43a85
Not sure if all these are smart state scans, and if all these are potential targets for MiqSchedule.
OK, indeed seems the only suspect except ContainerImage is EmsCluster:
https://github.com/ManageIQ/manageiq/blob/142129ed/app/models/ems_cluster.rb#L212-L220
Anybody knows what that is (and what's save_drift_state)?
Can/should we add an optional ignored userid arg to it?

Copy link
Member

Choose a reason for hiding this comment

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

Can/should we add an optional ignored userid arg to it?

Yeah I think so, all of the other scan methods are def scan(userid = "system", options = {}) so this would keep things consistent.

Anybody knows what that is (and what's save_drift_state)?

http://manageiq.org/docs/reference/fine/doc-Managing_Infrastructure_and_Inventory/miq/#detecting-drift-on-clusters
I'm not sure if this can be scheduled but it can be run from the UI.

"the best way to get the right answer on the internet is not to ask a question; it's to post the wrong answer."

LOL that is brilliant

Copy link
Contributor Author

@nimrodshn nimrodshn Apr 10, 2018

Choose a reason for hiding this comment

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

@agrare just to be sure were talking about scan in container_image.rb?

Copy link
Member

Choose a reason for hiding this comment

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

And EmsCluster#scan

@agrare agrare self-assigned this Apr 9, 2018
@nimrodshn nimrodshn force-pushed the add_scan_with_userid branch 2 times, most recently from 3f45458 to d84be1c Compare April 10, 2018 13:34
@nimrodshn
Copy link
Contributor Author

@agrare @cben fixed 👍 PTAL.

propagate userid

appease linter

adding userid param to EmsCluster#scan and ContainerImage#scan

appeasing linter
@miq-bot
Copy link
Member

miq-bot commented Apr 10, 2018

Checked commit nimrodshn@af34984 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

Looks good.

@agrare agrare merged commit 959fcd6 into ManageIQ:master Apr 10, 2018
@agrare agrare added this to the Sprint 84 Ending Apr 23, 2018 milestone Apr 10, 2018
@JPrause
Copy link
Member

JPrause commented Apr 10, 2018

@miq-bot add_label blocker

simaishi pushed a commit that referenced this pull request Apr 12, 2018
Adding scan action with userid to container_image
(cherry picked from commit 959fcd6)

https://bugzilla.redhat.com/show_bug.cgi?id=1566529
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 84d84f380fa6924549c652132ebab353a4f10419
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Apr 10 12:14:30 2018 -0400

    Merge pull request #17264 from nimrodshn/add_scan_with_userid
    
    Adding scan action with userid to container_image
    (cherry picked from commit 959fcd687af3b4fa44ee3061c5c77fcd1926d529)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1566529

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants