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

[WIP] Support mapped tags #18046

Closed
wants to merge 2 commits into from
Closed

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Oct 2, 2018

This PR allows to use tags, categories(Classification + Tag model) to be usable in UI.
These PRs modify queries to display tags and categories(Classifications) with read_only=true flag and together with name with prefix of ContainerLabelTagMapping::TAG_PREFIXES (mapped labels) and from others regions. So this can be used on global region.

Briefly, user can use mapped tags from remote regions on global region.

Scenario

Let's assume you have mapped tags on remote region:

screenshot 2018-10-02 at 10 31 36

then after replication you can use them in for reporting, assigning and expresion editors:
screenshot 2018-10-02 at 10 35 33

screenshot 2018-10-02 at 10 37 05

report result with mapped tags:

screenshot 2018-10-02 at 10 38 06

it is also usable in advanced searches.

@miq-bot add_label enhancement

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1526000
#18015 (first part of 3)
PR is second part of 3

@miq-bot assign @gtanzillo

Method get_entry_details and MiqExpression#categories are used for listing mapped tag in expression editor
in UI. https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/views/layouts/exp_atom/_edit_tags.html.haml#L27

We need to look for Classifiication object also in other regions and we
need to find just if tag with proper name exist at least in any region.
@miq-bot
Copy link
Member

miq-bot commented Oct 2, 2018

Checked commits lpichler/manageiq@512ecde~...c9d9344 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

@@ -46,6 +46,27 @@ class Classification < ApplicationRecord

FIXTURE_FILE = FIXTURE_DIR.join("classifications.yml")

def self.parent_classifications
Copy link
Member

Choose a reason for hiding this comment

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

The existing method categories does a similar thing but is scoped to the current region. Perhaps you can modify that method to get all the categories across all regions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the method categories lists (with these changes) categories with read_only=true && mapped categories from other regions. So it would not help in UI to display them in tag assignments.

@gtanzillo gtanzillo added the wip label Oct 9, 2018
@gtanzillo gtanzillo changed the title Support mapped tags [WIP] Support mapped tags Oct 9, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 7, 2019

This pull request is not mergeable. Please rebase and repush.

cats = where(:classifications => {:parent_id => 0}).includes(:tag, :children)
cats = cats.in_region(region_id) if region_id
cats.select { |c| c.ns == ns }
classificatin_scope = Classification.parent_classifications.references(:tag).includes(:tag, :children)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: s/classificatin/classification/g

@@ -46,6 +46,27 @@ class Classification < ApplicationRecord

FIXTURE_FILE = FIXTURE_DIR.join("classifications.yml")

def self.parent_classifications
where(:classifications => {:parent_id => 0})
Copy link
Contributor

Choose a reason for hiding this comment

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

This will change from 0 to nil in #18301. Consider abstracting this using is_category scope.

condition_for_mapped_tags = ContainerLabelTagMapping::TAG_PREFIXES.map { "tags.name LIKE ?" }.join(' OR ')

tag_values = ContainerLabelTagMapping::TAG_PREFIXES.map { |x| "#{x}%:%" }
where(conditions).parent_classifications.includes(:tag).references(:tag).where(condition_for_mapped_tags, *tag_values).each do |c|
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this use joins(:tag).merge(Tag.controlled_by_mapping) instead of directly querying on TAG_PREFIXES?
I'd like to not leak the fact it's currently implemented as prefix-based query

@lpichler
Copy link
Contributor Author

@cben thank you very much for review I appreciated it but it is not needed it, I will back to these PRs when we will needed it. closing

@lpichler lpichler closed this Jan 25, 2019
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.

4 participants