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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify tag list reconstruct #12670

Merged
merged 4 commits into from Nov 29, 2016

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Nov 16, 2016

continuing of #12608 (created on top of this PR - reason for WIP)

馃巵 @isimluk

@miq-bot add_label wip
@miq-bot add_label chargeback, refactoring

@miq-bot miq-bot added the wip label Nov 16, 2016
@lpichler lpichler force-pushed the simplify_tag_list_reconstruct branch 2 times, most recently from 33aa471 to 8629121 Compare November 16, 2016 12:04
@@ -21,11 +21,11 @@ def tag_prefix
def tag_list_reconstruct
tag_list = tag_names

if resource_type == Container.name
if resource.instance_of?(Container)
Copy link
Member

Choose a reason for hiding this comment

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

I think this may lead to extra Select query.

Although, this is minor nitpick and can be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about it I thought that resource is preloaded in by include base_rollup query, isn't ?

https://github.com/ManageIQ/manageiq/blob/master/app/models/chargeback.rb#L26

if resource.instance_of?(Container)
state = resource.vim_performance_state_for_ts(timestamp.to_s)
tag_list += state.image_tag_names if state.present?
image_tag_name = "#{state.try(:image_tag_name)}|" if state
Copy link
Member

Choose a reason for hiding this comment

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

I think either that if or that try is enough. I think, there is no need to have both.

This is however minor nitpick and can be ignored.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I think we also avoid to use try because then you can easily overlook that there is typo

@isimluk
Copy link
Member

isimluk commented Nov 17, 2016

馃憤 Thanks!

@miq-bot
Copy link
Member

miq-bot commented Nov 26, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@isimluk
Copy link
Member

isimluk commented Nov 27, 2016

Rates cache is now in. We can rebase and unwip. 馃崝

@lpichler lpichler force-pushed the simplify_tag_list_reconstruct branch from 8629121 to 48363fb Compare November 28, 2016 08:48
@lpichler lpichler force-pushed the simplify_tag_list_reconstruct branch from 48363fb to 70bd693 Compare November 28, 2016 09:24
@miq-bot
Copy link
Member

miq-bot commented Nov 28, 2016

Checked commits lpichler/manageiq@2009cc4~...70bd693 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 馃崻

@lpichler
Copy link
Contributor Author

@miq-bot remove_label wip

@lpichler lpichler changed the title [WIP] Simplify tag list reconstruct Simplify tag list reconstruct Nov 28, 2016
@miq-bot miq-bot removed the wip label Nov 28, 2016
@gtanzillo
Copy link
Member

馃憤 LGTM

@gtanzillo gtanzillo added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 29, 2016
@gtanzillo gtanzillo merged commit 9154d8d into ManageIQ:master Nov 29, 2016
@simaishi
Copy link
Contributor

Backported to Euwe via #13419

@lpichler lpichler deleted the simplify_tag_list_reconstruct branch March 3, 2017 09:42
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.

None yet

6 participants