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

add relation between container projects and persistent volume claims #15932

Merged
merged 1 commit into from
Sep 13, 2017

Conversation

zeari
Copy link

@zeari zeari commented Sep 4, 2017

persistent_volume_claims are under a namespace\project. This PR adds the relation.
depends on ManageIQ/manageiq-schema#60 (merged)

@zeari
Copy link
Author

zeari commented Sep 4, 2017

@zakiva Please review
cc @simon3z @moolitayer @cben

@miq-bot add_label providers/containers

@cben
Copy link
Contributor

cben commented Sep 5, 2017

Please add test for this relation in refresher_spec.
You might have to also adjust graph refresh.

Sorry, you already did in ManageIQ/manageiq-providers-kubernetes#108

Copy link

@moolitayer moolitayer 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. nit comment

@@ -42,9 +42,12 @@ def save_persistent_volume_claims_inventory(ems, hashes)
return if hashes.nil?

ems.persistent_volume_claims.reset
hashes.each do |h|
h[:container_project_id] = h.fetch_path(:project, :id)
Copy link

@moolitayer moolitayer Sep 5, 2017

Choose a reason for hiding this comment

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

How about:

h[:container_project_id] = h.delete_path(:project)[:id]

and remove the ignored key from line 50. Not a big deal, to me it makes the parsing more self contained in terms of the hashes we output) I think we use both strategies now.

@zeari zeari force-pushed the add_project_id_to_volume_claims branch from a87e611 to d66e647 Compare September 5, 2017 10:13
@zeari
Copy link
Author

zeari commented Sep 5, 2017

Looks good. nit comment

@moolitayer Fixed

@miq-bot
Copy link
Member

miq-bot commented Sep 5, 2017

Checked commit zeari@d66e647 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@@ -42,6 +42,9 @@ def save_persistent_volume_claims_inventory(ems, hashes)
return if hashes.nil?

ems.persistent_volume_claims.reset
hashes.each do |h|
h[:container_project_id] = h.delete_path(:project)[:id]
Copy link
Contributor

Choose a reason for hiding this comment

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

can use just delete here. doesn't matter.

Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@zeari
Copy link
Author

zeari commented Sep 11, 2017

@Fryguy @blomquisg ManageIQ/manageiq-schema#60 was merged. Can we merge this one too?

@blomquisg blomquisg merged commit 05cd055 into ManageIQ:master Sep 13, 2017
@blomquisg blomquisg added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 13, 2017
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.

5 participants