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

Save persistent volume claim to project relation #108

Merged
merged 1 commit into from
Sep 15, 2017

Conversation

zeari
Copy link

@zeari zeari commented Sep 4, 2017

depends on:
ManageIQ/manageiq-schema#60
ManageIQ/manageiq#15932

@cben @moolitayer @zakiva Please review
cc @simon3z we are going to need this relation for chargeback so we can identify pvc's that are not connected to pods but are connected to a project

@simon3z simon3z self-assigned this Sep 4, 2017
@simon3z simon3z requested a review from cben September 4, 2017 14:01
@zeari zeari force-pushed the add_project_id_to_volume_claims branch from 6f51da0 to 78a5870 Compare September 4, 2017 15:42
@miq-bot
Copy link
Member

miq-bot commented Sep 4, 2017

Checked commit zeari@78a5870 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@@ -189,6 +189,7 @@ def get_persistent_volume_claims(inventory)
process_collection(inventory["persistent_volume_claim"], key) { |n| parse_persistent_volume_claim(n) }
@data[key].each do |pvc|
@data_index.store_path(key, :by_namespace_and_name, pvc[:namespace], pvc[:name], pvc)
pvc[:project] = @data_index.fetch_path(path_for_entity("namespace"), :by_name, pvc[:namespace])
Copy link
Contributor

Choose a reason for hiding this comment

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

Much of the parser sets :project instead of :container_project for some reason. I want to fix it one day.
Is it easy to do :container_project here? Or do you think it's better to keep the pattern?

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.

Much of the parser sets :project instead of :container_project for some reason. I want to fix it one day.

it's been bugging me too :) never got around to fixing it.

Copy link
Author

Choose a reason for hiding this comment

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

keep the pattern. change it in the next refactor.

@@ -371,6 +372,7 @@ def get_persistent_volume_claims_graph(inv)

inv["persistent_volume_claim"].each do |pvc|
h = parse_persistent_volume_claim(pvc)
h[:container_project] = lazy_find_project(:name => h[:namespace])

Choose a reason for hiding this comment

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

@cben so this has to be done in both refreshes since it involves data_index right? in cases where we change parsing (e.g add another textual field to an entity) we will have to update only the parse method?

Copy link
Contributor

@cben cben Sep 5, 2017

Choose a reason for hiding this comment

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

Yeah. A column on same table can be set once in parse_* method.

Links to other tables are done differently in 2 refreshes.

  • The old refresh does links directly by nested hashes. sometimes it's implicit, nesting comes from parse_* or directly from kubernetes. nesting across API requests is constructed via data_index.
    Then, in some cases depending on saving order, save_inventory needs to break nestings and set ids (this is the case add relation between container projects and persistent volume claims manageiq#15932).
  • Graph refresh does links within API request directly by linking InventoryObject, across API requests by lazy_find. (images & registries still unrefactored via @data_index.)
    Graph saving figures order automatically, no manual breaking needed.

@moolitayer
Copy link

moolitayer commented Sep 5, 2017

Looks good, travis failing.

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

Travis fail is because schema & core deps not merged yet.

@moolitayer
Copy link

moolitayer commented Sep 5, 2017

Yes of course ¯_(ツ)_/¯

@zeari zeari closed this Sep 13, 2017
@zeari zeari reopened this Sep 13, 2017
@zeari
Copy link
Author

zeari commented Sep 14, 2017

All tests green @moolitayer @cben @simon3z

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 👍

@cben
Copy link
Contributor

cben commented Sep 14, 2017

@moolitayer You're 2nd approver, and have merge powers, hint hint :)

@moolitayer
Copy link

@simon3z wants to review this one @cben

@simon3z simon3z merged commit eea3322 into ManageIQ:master Sep 15, 2017
@agrare agrare added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 19, 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.

6 participants