Adding openshift_container_images #23

enoodle · 2017-06-15T13:41:24Z

Addes openshift_container_images to the openshift provider.
Adding parsing for ems_ref for images originating from Openshift.

based on: ManageIQ/manageiq#15386

moolitayer · 2017-06-19T10:06:48Z

app/models/manageiq/providers/openshift/container_image.rb

@@ -0,0 +1,13 @@
+class ManageIQ::Providers::Openshift::ContainerImage < ContainerImage


class ManageIQ::Providers::Openshift::ContainerManager::ContainerImage < ContainerImage
Also change path

enoodle · 2017-06-19T12:15:42Z

@moolitayer done

moolitayer · 2017-06-19T14:37:07Z

app/models/manageiq/providers/openshift/container_manager.rb

@@ -8,6 +8,8 @@ class ManageIQ::Providers::Openshift::ContainerManager < ManageIQ::Providers::Co
  require_nested :RefreshWorker
  require_nested :Refresher

+  has_many :openshift_container_images, :foreign_key => :ems_id, :dependent => :destroy


We don't need this now right?

No, it is just nice to have (I need to add ":class_name => ..." there though) do you want me to remove this?

moolitayer · 2017-06-19T15:05:55Z

app/models/manageiq/providers/openshift/container_manager.rb

@@ -8,6 +8,9 @@ class ManageIQ::Providers::Openshift::ContainerManager < ManageIQ::Providers::Co
  require_nested :RefreshWorker
  require_nested :Refresher

+  has_many :openshift_container_images, :foreign_key => :ems_id, :dependent => :destroy,
+    :class_name => 'ManageIQ::Providers::Openshift::ContainerManager::ContainerImage'


Yes please remove unless you have a solid use case

moolitayer

LTGM 👍

enoodle · 2017-06-19T15:23:47Z

This will require test restart after the base PR ManageIQ/manageiq#15386 is merged

Fryguy · 2017-07-03T16:00:51Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

@@ -0,0 +1,13 @@
+class ManageIQ::Providers::Openshift::ContainerManager::ContainerImage < ContainerImage
+  acts_as_miq_taggable
+  include NewWithTypeStiMixin


These two lines should not be here. They both belong in the base ContainerImage.

Fryguy · 2017-07-03T18:04:57Z

ManageIQ/manageiq#15386 is merged, so rekicking travis

simon3z · 2017-07-04T08:56:37Z

app/models/manageiq/providers/openshift/container_manager/refresh_parser.rb

@@ -175,6 +175,9 @@ def parse_openshift_image(openshift_image)
        ref = "#{ContainerImage::DOCKER_PULLABLE_PREFIX}#{id}"
        new_result = parse_container_image(id, ref)

+        #new_result[:ems_ref] = openshift_image.metadata.uid


@enoodle do we need this comment?

@simon3z No, I commented this out because of ManageIQ/manageiq-schema#21 (comment)

Do you think we should remove this column or put the digest there? Anything else we can do?

what do you use the ems_ref for?

enoodle · 2017-07-10T11:15:04Z

I removed the processing of ems_ref from this PR, and created ManageIQ/manageiq-schema#38 to remove this column. @moolitayer @Fryguy @simon3z PTAL.

enoodle · 2017-07-13T17:16:45Z

@moolitayer @simon3z
ManageIQ/manageiq-schema#38 seems not to be an issue, as this current PR is not touching ems_ref.
BUT, This PR now coupled with ManageIQ/manageiq#15519. These two PRs should be merged together as this one will introduce a problem with SSA for containers that ManageIQ/manageiq#15519 will solve, but due to the tests in ManageIQ/manageiq#15519 it needs this one to be green. Any advice?

moolitayer · 2017-07-13T17:36:06Z

@enoodle I didn't understand. On what is ManageIQ/manageiq#15519 dependent?

enoodle · 2017-07-13T17:43:40Z

@moolitayer The tests in ManageIQ/manageiq#15519 are red because they need the container image class definition from this PR.

lpichler · 2017-07-14T14:05:00Z

Note:
without this PR and after migrating records by this migration

AR queries on ContainerImage ( with type=ManageIQ::Providers::Openshift::ContainerManager::ContainerImage) are failing.

ContainerImage.first =>
ActiveRecord::SubclassNotFound: The single-table inheritance mechanism failed to locate the 
subclass: 'ManageIQ::Providers::Openshift::ContainerManager::ContainerImage'.
 This error is raised because the column 'type' is reserved for storing the class in case of 
inheritance.
 Please rename this column if you didn't intend it to be used for storing the inheritance class or overwrite ContainerImage.inheritance_column to use another column for that information.

This is also fixing error about missing `ManageIQ::Providers::Openshift::ContainerManager::ContainerImage` ManageIQ/manageiq-providers-openshift#23 (comment)

moolitayer · 2017-07-16T09:56:37Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

@@ -0,0 +1,12 @@
+class ManageIQ::Providers::Openshift::ContainerManager::ContainerImage < ContainerImage
+  acts_as_miq_taggable


Having this in the base class is not enough?

moolitayer · 2017-07-16T09:57:21Z

app/models/manageiq/providers/openshift/container_manager/container_image.rb

+      digest,
+      "security.manageiq.org/failed-policy" => causing_policy,
+      "images.openshift.io/deny-execution"  => "true"
+    )


We need a PR removing this method from manageiq/manageiq

ManageIQ/manageiq#15386 it is already merged.

miq-bot · 2017-07-16T11:34:00Z

Checked commit enoodle@ad5ac8a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

simon3z · 2017-07-17T16:16:03Z

@Fryguy you requested changes here. Can you check if it's OK now and update your review status? Thanks!

My concerns were removed

simon3z · 2017-07-20T22:20:59Z

My concerns were removed

Thanks @Fryguy !

enoodle changed the title ~~Processing for OpenshiftContainerImage~~ [WIP] Processing for OpenshiftContainerImage Jun 15, 2017

miq-bot added the wip label Jun 15, 2017

enoodle force-pushed the openshift_container_image_class_processing branch from 4f18efe to 4410d13 Compare June 15, 2017 13:53

enoodle changed the title ~~[WIP] Processing for OpenshiftContainerImage~~ Adding openshift_container_images Jun 15, 2017

enoodle force-pushed the openshift_container_image_class_processing branch from 4410d13 to f04f0e1 Compare June 15, 2017 13:55

miq-bot removed the wip label Jun 15, 2017

enoodle force-pushed the openshift_container_image_class_processing branch 3 times, most recently from 6c2aa9f to dfacf0e Compare June 18, 2017 13:19

enoodle mentioned this pull request Jun 18, 2017

Adding ContainerImage subclasses ManageIQ/manageiq#15386

Merged

moolitayer reviewed Jun 19, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_processing branch 2 times, most recently from b935586 to 7119e22 Compare June 19, 2017 12:15

moolitayer reviewed Jun 19, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_processing branch from 7119e22 to cdad41e Compare June 19, 2017 14:55

moolitayer reviewed Jun 19, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_processing branch from cdad41e to 6488d0f Compare June 19, 2017 15:14

moolitayer approved these changes Jun 19, 2017

View reviewed changes

Fryguy approved these changes Jun 19, 2017

View reviewed changes

enoodle force-pushed the openshift_container_image_class_processing branch from 6488d0f to 630b14f Compare June 20, 2017 14:29

Fryguy previously requested changes Jul 3, 2017

View reviewed changes

Fryguy closed this Jul 3, 2017

Fryguy reopened this Jul 3, 2017

This was referenced Jul 4, 2017

Adding sti mixin to container_image base class ManageIQ/manageiq#15505

Merged

Container ssa annotate success ManageIQ/manageiq#15031

Merged

simon3z reviewed Jul 4, 2017

View reviewed changes

enoodle mentioned this pull request Jul 5, 2017

subclassing ContainerImage to Openshift::ContainerImage ManageIQ/manageiq-schema#21

Merged

enoodle mentioned this pull request Jul 6, 2017

JobProxyDispatcher should use all container image classes ManageIQ/manageiq#15519

Merged

enoodle force-pushed the openshift_container_image_class_processing branch from dc8a1d9 to 6a848e3 Compare July 10, 2017 10:59

lpichler mentioned this pull request Jul 14, 2017

Fix issue in report definition for chargeback ManageIQ/manageiq-ui-classic#1693

Merged

moolitayer reviewed Jul 16, 2017

View reviewed changes

add openshift_container_images to openshift provider

ad5ac8a

enoodle force-pushed the openshift_container_image_class_processing branch from 6a848e3 to ad5ac8a Compare July 16, 2017 11:32

This was referenced Jul 20, 2017

ActiveRecord::SubclassNotFound for ContainerImage ManageIQ/manageiq-providers-kubernetes#70

Closed

Move Openshift specific code into its provider directory. ManageIQ/manageiq#15523

Merged

simon3z merged commit 8f1ab78 into ManageIQ:master Jul 20, 2017

moolitayer added this to the Sprint 65 Ending Jul 24, 2017 milestone Aug 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding openshift_container_images #23

Adding openshift_container_images #23

enoodle commented Jun 15, 2017 •

edited

Loading

moolitayer Jun 19, 2017

enoodle commented Jun 19, 2017

moolitayer Jun 19, 2017 •

edited

Loading

enoodle Jun 19, 2017

moolitayer Jun 19, 2017

moolitayer left a comment

enoodle commented Jun 19, 2017

Fryguy Jul 3, 2017

Fryguy commented Jul 3, 2017

simon3z Jul 4, 2017

enoodle Jul 4, 2017

moolitayer Jul 4, 2017

enoodle commented Jul 10, 2017

enoodle commented Jul 13, 2017

moolitayer commented Jul 13, 2017

enoodle commented Jul 13, 2017

lpichler commented Jul 14, 2017

moolitayer Jul 16, 2017

moolitayer Jul 16, 2017

enoodle Jul 16, 2017

miq-bot commented Jul 16, 2017

simon3z commented Jul 17, 2017

simon3z commented Jul 20, 2017

		@@ -0,0 +1,13 @@
		class ManageIQ::Providers::Openshift::ContainerImage < ContainerImage

		@@ -0,0 +1,12 @@
		class ManageIQ::Providers::Openshift::ContainerManager::ContainerImage < ContainerImage
		acts_as_miq_taggable

Adding openshift_container_images #23

Adding openshift_container_images #23

Conversation

enoodle commented Jun 15, 2017 • edited Loading

Choose a reason for hiding this comment

enoodle commented Jun 19, 2017

moolitayer Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer left a comment

Choose a reason for hiding this comment

enoodle commented Jun 19, 2017

Choose a reason for hiding this comment

Fryguy commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enoodle commented Jul 10, 2017

enoodle commented Jul 13, 2017

moolitayer commented Jul 13, 2017

enoodle commented Jul 13, 2017

lpichler commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 16, 2017

simon3z commented Jul 17, 2017

simon3z commented Jul 20, 2017

enoodle commented Jun 15, 2017 •

edited

Loading

moolitayer Jun 19, 2017 •

edited

Loading