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 UI support for Pod to PersistentVolume relationship. #3299

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

nimrodshn
Copy link
Contributor

@nimrodshn nimrodshn commented Jan 23, 2018

Adding UI support for Pod to PersistentVolume relationship.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1435235
GIF:
pod_to_pv
cc: @himdel @kbrock @zeari

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jan 23, 2018

@himdel @kbrock the reason I had to add "PersistentVolume" => "persistent_volumes" to HAS_ASSOCIATION was that otherwise get_parent_targets in miq_report/search.rb would derive it from the base class (see get_parent_targets line 67 [1]), which is ContainerVolumes, and that would lead to an empty report.

Credit to @kbrock for finding this and saving my life 🥇

[1]https://github.com/ManageIQ/manageiq/blob/d992f980a857db789c8aa8b3636bf1b6559ca539/app/models/miq_report/search.rb#L67

@nimrodshn
Copy link
Contributor Author

@miq-bot add_label gaprindashvili/yes, providers/containers, bug

@nimrodshn nimrodshn force-pushed the support_pv_to_pod_relationship branch 2 times, most recently from 940bb69 to 812e779 Compare January 23, 2018 09:17
@nimrodshn nimrodshn changed the title Adding UI support for PersistentVolume to Pod relationship. Adding UI support for Pod to PersistentVolume relationship. Jan 23, 2018
@@ -16,7 +16,7 @@ def textual_group_relationships
_("Relationships"),
%i(
ems container_project container_services container_replicator containers container_node
lives_on container_images
lives_on container_images pod_to_persistent_volume_relationship
Copy link

Choose a reason for hiding this comment

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

How about persistent_volumes instead 😅

@nimrodshn nimrodshn force-pushed the support_pv_to_pod_relationship branch from 812e779 to 9167739 Compare January 23, 2018 11:07
@nimrodshn
Copy link
Contributor Author

@zeari fixed. 👍

Copy link
Member

@kbrock kbrock 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.
Glad you found the hash that was causing havoc on the search.rb code. very good find.

I don't know too much about the ui code - so I'm not sure if my 👍 means anything

@@ -171,7 +171,8 @@ def textual_persistent_volume
end

def textual_persistent_volumes
textual_link(@record.persistent_volumes)
link = url_for_only_path(:id => @record.id, :action => "show", :display => "persistent_volumes")
Copy link
Member

Choose a reason for hiding this comment

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

did you still need to use url_for_only_path now that you taught the system how to deal with persistent_volumes? um, did you test if the url "just works"?

@nimrodshn nimrodshn force-pushed the support_pv_to_pod_relationship branch from 9167739 to 5a9ce8f Compare January 28, 2018 12:34
@@ -36,3 +36,10 @@
= multiple_relationship_link(@record, :container)
= multiple_relationship_link(@record, :container_image)
= single_relationship_link(@record, :container_node)
- if @record.number_of(:persistent_volumes).zero?
Copy link
Contributor

@himdel himdel Jan 29, 2018

Choose a reason for hiding this comment

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

This is wrong, you need to use one of the *relationship_link functions (those also check the user is allowed to see the thing..)

Any reason you can't do that?

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Feb 6, 2018

@himdel fixed - but in order for the link to work to work I had to add some line to miq_product_features.yml so now this is pending ManageIQ/manageiq#16956 😢
screenshot from 2018-02-06 11-40-53

@@ -36,3 +36,4 @@
= multiple_relationship_link(@record, :container)
= multiple_relationship_link(@record, :container_image)
= single_relationship_link(@record, :container_node)
= multiple_relationship_link(@record, :persistent_volumes)
Copy link
Contributor

@himdel himdel Feb 7, 2018

Choose a reason for hiding this comment

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

:persistent_volume .. this should make multiple_relationship_link use the right rbac feature, while still accessing the right data (as it does call table_name.pluralize for that).

(so no need for the other pr after all, I think)

Adding UI support for pv to pod relationship

adding PersistentVolume to HAS_ASSOCATION

refactor code

adding relationship to navbar

refactoring textual_persistent_volumes

works without the link

fixed listnav

fixing listanv
@nimrodshn nimrodshn force-pushed the support_pv_to_pod_relationship branch from f586d16 to 909739e Compare February 7, 2018 14:11
@nimrodshn
Copy link
Contributor Author

@himdel thanks mate!

@miq-bot
Copy link
Member

miq-bot commented Feb 7, 2018

Checked commit nimrodshn@909739e with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

@nimrodshn
Copy link
Contributor Author

@himdel PTAL?

@himdel himdel self-assigned this Feb 8, 2018
@himdel himdel merged commit 2341a39 into ManageIQ:master Feb 8, 2018
@himdel
Copy link
Contributor

himdel commented Feb 8, 2018

LGTM, verified in the UI :)

@himdel himdel added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 8, 2018
simaishi pushed a commit that referenced this pull request Mar 7, 2018
Adding UI support for Pod to PersistentVolume relationship.
(cherry picked from commit 2341a39)

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

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit 5bac531ee00dfcfe4e383e7d61c3215db5da1675
Author: Martin Hradil <[email protected]>
Date:   Thu Feb 8 17:07:37 2018 +0100

    Merge pull request #3299 from nimrodshn/support_pv_to_pod_relationship
    
    Adding UI support for Pod to PersistentVolume relationship.
    (cherry picked from commit 2341a397a870611ed0d0de5019c7f29017b150e1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552889

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.

6 participants