Image scanning: Add image name to task name #105

zakiva · 2017-08-29T14:13:58Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1483590

Related PR: #119

Adding the scanned image name to the task name:

zakiva · 2017-08-29T14:17:23Z

@simon3z @moolitayer Please review

simon3z · 2017-08-29T14:20:39Z

@zakiva please check if this is OK for Einat as well, she needs this for her tests.

moolitayer · 2017-08-30T08:59:09Z

@zakiva can we avoid reloading the image by passing it's name in check_policy_prevent?

zakiva · 2017-08-30T11:20:41Z

app/models/manageiq/providers/kubernetes/container_manager_mixin.rb

@@ -175,11 +175,12 @@ def scan_job_create(entity)

  def raw_scan_job_create(target_class, target_id = nil, userid = nil)
    target_class, target_id = target_class.class.name, target_class.id if target_class.kind_of?(ContainerImage)


@moolitayer do you know in what cases this check is relevant? I might need to update the target_name as well in case we pass it through check_policy_prevent

So this is a little confusing:
we used to pass an actual image object - not class and id. Then we wanted to move to class and id (to prevent queue serialization) but we wanted to be backward compatible with any queue items created by the old code that remain after upgrade.

Can you add as part of this PR a comment above this line

# maintain backward compatibility pre https://github.com/ManageIQ/manageiq/pull/13722

(for the next person reading this code)

@moolitayer thanks, done

moolitayer · 2017-08-30T11:21:33Z

@zakiva please check if this is OK for Einat as well, she needs this for her tests.

cc @epacific1, changes are coming ❄️

zakiva · 2017-08-30T11:22:32Z

@zakiva can we avoid reloading the image by passing it's name in check_policy_prevent?

@moolitayer yes, I think it's possible. Please see a small related question in line.

zakiva · 2017-09-12T16:26:01Z

@moolitayer PTAL

moolitayer

LGTM 👍
@simon3z @cben please review this change

cben

nice 👍

simon3z · 2017-09-12T20:13:22Z

app/models/manageiq/providers/kubernetes/container_manager_mixin.rb

-    target_class, target_id = target_class.class.name, target_class.id if target_class.kind_of?(ContainerImage)
+  def raw_scan_job_create(target_class, target_id = nil, userid = nil, target_name = nil)
+    # maintain backward compatibility pre https://github.com/ManageIQ/manageiq/pull/13722
+    if target_class.kind_of?(ContainerImage)


@moolitayer @cben can't we drop this entirely for 5.9? Maybe we can fail if target_class is an instance instead of a class name.

we can if you decide we can ;-)
Half a year passed since the code stopped queueing AR instances, but for a user upgrading from last release, it's still simultaneous queueing & dequeing side change, they'll might have some scans with AR instances queued.
IMHO it's no big deal to drop a few scans.
Or could keep this compat in Gaprindashvili release and drop entirely in H release...

@moolitayer @cben if this is here to be backward-compatible with 4.2 then we can drop it in 4.6 (again, let's fail gracefully with a meaningful error message).

Let me know if I am missing something (e.g. version assumptions are incorrect, etc.).

Oh you're right, I thought it didn't make it into fine, but fine-1 was already queueing event_target.class.name, event_target.id and dequeuing with defensive if target_class.kind_of?(ContainerImage).

[upstream cheatsheet for this conversation: 5.9 & 4.6 both refer to master/Gaprindashvili, 4.5 to Fine, 4.2 to Euwe.]

This fix is targeted for 4.5 zstream. Can we keep the check in this one for simplicity and remove it from master? (@simon3z maybe that is what you are suggesting but I'm not sure)

@simon3z We can merge this PR to master as is and backport it. Then, I will send a separate PR to drop this check on master.

@zakiva 👍 Can you please send the second PR and label it as fine/no so we can view the while solution?

epacific1 · 2017-09-17T12:29:55Z

Seems that there is no way to uniquely identify the image from the task list. If that indeed is the case, then, this seems to be the next best solution.

zakiva · 2017-09-17T13:02:15Z

@epacific1 I added the image id too as we discussed, please see the updated screenshot above

epacific1 · 2017-09-18T11:38:16Z

I had a look at the new screenshot. It does not make sense to display the ID as (xx).
Lets revert back to just the image name.

zakiva · 2017-09-18T14:12:31Z

I had a look at the new screenshot. It does not make sense to display the ID as (xx).
Lets revert back to just the image name.

@epacific1 OK, done.

miq-bot · 2017-09-18T14:14:19Z

Checked commit zakiva@cf53509 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

simaishi · 2017-11-10T18:20:22Z

Fine backport (to manageiq repo) details:

$ git log -1
commit 0f30b0a104d6473bffc9ac860aed71a43a01b2d5
Author: Federico Simoncelli <[email protected]>
Date:   Wed Sep 27 23:39:09 2017 +0200

    Merge pull request #105 from zakiva/add_image_name
    
    Image scanning: Add image name to task name
    (cherry picked from commit 7c48e385e8d10f20adaf164e59fe3876fbd7b318)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1496943

zakiva commented Aug 30, 2017

View reviewed changes

zakiva force-pushed the add_image_name branch from 01d2295 to 3cdbf2d Compare September 4, 2017 11:23

simon3z assigned moolitayer Sep 4, 2017

simon3z added the enhancement label Sep 4, 2017

moolitayer approved these changes Sep 12, 2017

View reviewed changes

cben approved these changes Sep 12, 2017

View reviewed changes

simon3z reviewed Sep 12, 2017

View reviewed changes

moolitayer added the fine/yes label Sep 14, 2017

zakiva force-pushed the add_image_name branch from 3cdbf2d to 496438a Compare September 17, 2017 12:59

zakiva force-pushed the add_image_name branch from 496438a to cf53509 Compare September 18, 2017 14:10

Image scanning: Add image name to task name

cf53509

zakiva mentioned this pull request Sep 27, 2017

Fail on creating scanning job with image instance #119

Merged

cben approved these changes Sep 27, 2017

View reviewed changes

simon3z merged commit 7c48e38 into ManageIQ:master Sep 27, 2017

agrare added this to the Sprint 70 Ending Oct 2, 2017 milestone Oct 2, 2017

simaishi added fine/backported and removed fine/yes labels Nov 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image scanning: Add image name to task name #105

Image scanning: Add image name to task name #105

zakiva commented Aug 29, 2017 •

edited

Loading

zakiva commented Aug 29, 2017

simon3z commented Aug 29, 2017

moolitayer commented Aug 30, 2017

zakiva Aug 30, 2017 •

edited

Loading

moolitayer Aug 30, 2017 •

edited

Loading

zakiva Sep 4, 2017

moolitayer commented Aug 30, 2017

zakiva commented Aug 30, 2017

zakiva commented Sep 12, 2017

moolitayer left a comment

cben left a comment

simon3z Sep 12, 2017

cben Sep 12, 2017 •

edited

Loading

simon3z Sep 13, 2017

cben Sep 14, 2017 •

edited

Loading

moolitayer Sep 14, 2017

zakiva Sep 26, 2017 •

edited

Loading

moolitayer Sep 26, 2017

zakiva Sep 27, 2017

cben Sep 27, 2017

epacific1 commented Sep 17, 2017

zakiva commented Sep 17, 2017

epacific1 commented Sep 18, 2017

zakiva commented Sep 18, 2017

miq-bot commented Sep 18, 2017

simaishi commented Nov 10, 2017

		@@ -175,11 +175,12 @@ def scan_job_create(entity)

		def raw_scan_job_create(target_class, target_id = nil, userid = nil)
		target_class, target_id = target_class.class.name, target_class.id if target_class.kind_of?(ContainerImage)

Image scanning: Add image name to task name #105

Image scanning: Add image name to task name #105

Conversation

zakiva commented Aug 29, 2017 • edited Loading

zakiva commented Aug 29, 2017

simon3z commented Aug 29, 2017

moolitayer commented Aug 30, 2017

zakiva Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

zakiva Sep 4, 2017

Choose a reason for hiding this comment

moolitayer commented Aug 30, 2017

zakiva commented Aug 30, 2017

zakiva commented Sep 12, 2017

moolitayer left a comment

Choose a reason for hiding this comment

cben left a comment

Choose a reason for hiding this comment

simon3z Sep 12, 2017

Choose a reason for hiding this comment

cben Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

simon3z Sep 13, 2017

Choose a reason for hiding this comment

cben Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer Sep 14, 2017

Choose a reason for hiding this comment

zakiva Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer Sep 26, 2017

Choose a reason for hiding this comment

zakiva Sep 27, 2017

Choose a reason for hiding this comment

cben Sep 27, 2017

Choose a reason for hiding this comment

epacific1 commented Sep 17, 2017

zakiva commented Sep 17, 2017

epacific1 commented Sep 18, 2017

zakiva commented Sep 18, 2017

miq-bot commented Sep 18, 2017

simaishi commented Nov 10, 2017

zakiva commented Aug 29, 2017 •

edited

Loading

zakiva Aug 30, 2017 •

edited

Loading

moolitayer Aug 30, 2017 •

edited

Loading

cben Sep 12, 2017 •

edited

Loading

cben Sep 14, 2017 •

edited

Loading

zakiva Sep 26, 2017 •

edited

Loading