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.

Sign up for GitHub

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

Jump to bottom

Reports: Add Container entities to TAG_CLASSES #14535

Merged
merged 1 commit into from
Apr 18, 2017
Merged

Reports: Add Container entities to TAG_CLASSES #14535

merged 1 commit into from
Apr 18, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Mar 28, 2017 •
edited
Loading

@zakiva
Copy link
Contributor Author

zakiva commented Mar 28, 2017

@simon3z @zeari @cben Please review

@zakiva zakiva changed the title Reports: Add Container entites to TAG_CLASSES Reports: Add Container entities to TAG_CLASSES Mar 28, 2017
@zakiva zakiva force-pushed the fix_node_tags branch 2 times, most recently from 6fcc190 to 7c95ec1 Compare March 28, 2017 12:59
@zakiva
Copy link
Contributor Author

zakiva commented Mar 28, 2017

@miq-bot add_label providers/containers, bug

@zakiva
Copy link
Contributor Author

zakiva commented Mar 28, 2017

@miq-bot add_label reporting

@chessbyte chessbyte requested a review from simon3z March 28, 2017 17:30
cben
cben reviewed Mar 29, 2017
View reviewed changes
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.

Verified the entities against container models that acts_as_miq_taggable.
=> The only omission is ContainerDeploymentNode.
@dkorn should container_deployment_nodes table be available for reports?

lib/miq_expression.rb Outdated
'ContainerImageRegistry' => 'container_image_registry',
'ContainerRoute' => 'container_route',
'ContainerService' => 'container_service',
'ContainerTemplate' => 'container_template'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please sort these (incl. ContainerProject) alphabetically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks

@miq-bot
Copy link
Member

miq-bot commented Mar 29, 2017

Checked commit zakiva@b77338f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

@zeari
Copy link

zeari commented Mar 30, 2017

@isimluk @lpichler could you have a look?

@dkorn
Copy link
Contributor

dkorn commented Mar 30, 2017

Verified the entities against container models that acts_as_miq_taggable.
=> The only omission is ContainerDeploymentNode.
@dkorn should container_deployment_nodes table be available for reports?

@cben I can't think of a use case for the deployment nodes in a report.
Moreover, the feature is currently not used (hidden from the UI and undocumented in the API docs).

@zakiva
Copy link
Contributor Author

zakiva commented Apr 18, 2017

@gtanzillo Can you please take a look?

gtanzillo
gtanzillo approved these changes Apr 18, 2017
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gtanzillo gtanzillo added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 18, 2017
@gtanzillo gtanzillo merged commit bfc8cfa into ManageIQ:master Apr 18, 2017
@simaishi
Copy link
Contributor

As per BZ, I assume this should be fine/yes?

@isimluk
Copy link
Member

isimluk commented Apr 20, 2017

Yes, that's my understanding as well.

simaishi pushed a commit that referenced this pull request Apr 20, 2017
@gtanzillo @simaishi
Merge pull request #14535 from zakiva/fix_node_tags
6b97c0b 
Reports: Add Container entities to TAG_CLASSES
(cherry picked from commit bfc8cfa)

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

Fine backport details:

$ git log -1
commit 6b97c0bad11a9ff955adbdf0f7d600d3c7947dd1
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Tue Apr 18 14:27:41 2017 +0200

    Merge pull request #14535 from zakiva/fix_node_tags
    
    Reports: Add Container entities to TAG_CLASSES
    (cherry picked from commit bfc8cfa670e3c9047066164591b843d09999a6a2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1444067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeari zeari zeari approved these changes

@cben cben cben approved these changes

@gtanzillo gtanzillo gtanzillo approved these changes

@simon3z simon3z Awaiting requested review from simon3z

Assignees

@gtanzillo gtanzillo

Labels
bug core/reporting fine/backported providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zakiva @miq-bot @zeari @dkorn @simaishi @isimluk @gtanzillo @cben