Replace ContainerLabelTagMapping.controls_tag? with Tag.controlled_by_mapping scope #16449
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…_mapping scope More efficient, and can be used in graph refresh.
cben
force-pushed
the
mapped-tags-scope
branch
from
6fc96a2 to
c4e76ed
Compare
This was referenced Nov 12, 2017
cben
changed the title
cben
added a commit
to cben/manageiq-providers-kubernetes
that referenced
this pull request
Nov 13, 2017
Original code didn't set read_only (UI does set it), failed with new retagging implementation from ManageIQ/manageiq#16449
|
@cben Cannot apply the following label because they are not recognized: enhancement gaprindashvili/yes |
|
Checked commit cben@c4e76ed with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
Ladas
approved these changes
Nov 13, 2017
|
@miq-bot add-label performance, gaprindashvili/yes Yeah, I should benchmark. Wanted something that works at all first :-) |
|
Also, I hope later to move to simpler query on tag prefix, but not before some refactorings, that'd be g/no. |
agrare
approved these changes
Nov 13, 2017
23 tasks
cben
added a commit
to cben/manageiq-providers-kubernetes
that referenced
this pull request
Nov 13, 2017
- Use factory for more accurate test Original test didn't set read_only (UI does set it), failed with new retagging implementation from ManageIQ/manageiq#16449 - Test that we don't remove user-assigned tags
cben
added a commit
to cben/manageiq
that referenced
this pull request
Nov 13, 2017
It seems Tag.controlled_by_mapping works right but entity.tags.controlled_by_mapping doesn't (?) Until I debug it, this revert prevents regression of tag mapping in classic refresh (and specifically fixes save_tags_inventory_spec)
This was referenced Nov 13, 2017
cben
added a commit
to cben/manageiq
that referenced
this pull request
Nov 20, 2017
In ManageIQ#16453, I changed this spec to "realistically" create a tag mapping + category + tag. But I made a typo/thinko, taking the parent (category) tag to be assigned to the vm. This caused false failure for ManageIQ#16449 and days of lost work debugging/rewriting it, eventually caused similar failure in the rewrite too :-( https://bugzilla.redhat.com/show_bug.cgi?id=1508437
1 task
cben
added a commit
to cben/manageiq
that referenced
this pull request
Nov 20, 2017
A scope is needed for tag mapping in graph refresh, and may improve performance. Alternative to ManageIQ#16449 - this one no longer cares if there exists a ContainerLabelTagMapping linked to the category, only that it's name starts with `kubernetes:` / `amazon:`. This can happen to user-created categories too, so we also check it's read_only.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
THIS WAS REVERTED in #16462.
More efficient and necessary for ManageIQ/manageiq-providers-kubernetes#162 mapping in graph refresh.
Links
Overview/plan: ManageIQ/manageiq-providers-kubernetes#126
This is necessary to complete containers graph refresh, BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1470021