Add Pod to PersistentVolume relationship #15023
Conversation
|
@miq-bot add_label wip, providers/containers |
|
@zeari PTAL. |
nimrodshn
force-pushed
the
add_pv_to_node_realtionship
branch
from
f4e10e1
to
31ae27a
Compare
|
Added tests. |
nimrodshn
force-pushed
the
add_pv_to_node_realtionship
branch
from
39f684e
to
7f012f2
Compare
| @@ -26,6 +26,8 @@ class ContainerGroup < ApplicationRecord | |||
| belongs_to :old_container_project, :foreign_key => "old_container_project_id", :class_name => 'ContainerProject' | |||
| belongs_to :container_build_pod | |||
| has_many :container_volumes, :as => :parent, :dependent => :destroy | |||
| has_many :persistent_volume_claim, :through => :container_volumes | |||
| has_many :persistent_volumes, -> { where(:type=>'PersistentVolume') }, :through => :persistent_volume_claim, :source => :container_volumes | |||
There was a problem hiding this comment.
is this our :type or is it the type over in :persisted_volume_claim?
There was a problem hiding this comment.
is this our :type or is it the type over in :persisted_volume_claim?
@kbrock what do you mean by "our :type" vs "type over in :persistent_volume_claim?
Will add a test 👍
There was a problem hiding this comment.
@kbrock added tests for pvc relationship.
There was a problem hiding this comment.
Do we even need the where? I don't see any descendants of PersistentVolume
nimrodshn
force-pushed
the
add_pv_to_node_realtionship
branch
2 times, most recently
from
58fa178
to
e91ccfd
Compare
|
@nimrodshn @simon3z what is the status of this PR? |
|
@chessbyte It's still WIP, I'm working on the UI. might need to refactor this PR. |
refactoring
|
@zeari FYI |
|
@nimrodshn what is missing to make this final? (Remove WIP, etc.) |
|
@simon3z This part is done. I'm just stuck on some terrible bug in the UI 😢 (ManageIQ/manageiq-ui-classic#1774) , should I remove the WIP on this and continue work on the UI seperatly?. |
@nimrodshn yes please, get this reviewed properly (cc @moolitayer @zeari @zakiva) and remove the WIP. Thanks. |
nimrodshn
changed the title
|
@zakiva PTAL |
|
@nimrodshn |
spec/models/container_group.rb
Outdated
| expect(group.persistent_volume_claim.count).to eq(1) | ||
| expect(group.persistent_volumes.first.name).to eq("persistent_volume") | ||
| expect(group.persistent_volumes.count).to eq(1) | ||
| end |
There was a problem hiding this comment.
Since this is a one to many relationship i would expect the tests to have at least one more PV and to check that group.persistent_volumes contains both.
@zakiva Is there room for any 'play' from the provider side? like having claim without a PV or a claim for more than one PV?
nimrodshn
force-pushed
the
add_pv_to_node_realtionship
branch
from
e91ccfd
to
7acecd5
Compare
refactoring tests refactored tests refactored tests refactored some tests
nimrodshn
force-pushed
the
add_pv_to_node_realtionship
branch
from
7acecd5
to
be8ab10
Compare
|
Checked commits nimrodshn/manageiq@d56e359~...be8ab10 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
@zeari right now I can't think of a reason to report more than once the same PV in a ContainerGroup. Is it needed for Chargeback? (I don't think we need to account it twice) Let me know if I am missing something. LGTM 👍 ready for merge |
|
@blomquisg please review/merge. 😇 |
|
@chessbyte this has been reviewed and waiting to be merged by core team. |
|
@moolitayer 25 days of inactivity here is a little to much, can you add this to the list of the PRs pending review from core? |
|
@simon3z Added |
Currently we do not show which PersisentVolumes a given Pod is using -
In this PR we work towards supplying that information to the user.
cc: @zakiva @simon3z @kbrock
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1435235