ContainerLabelTagMapping::Mapper class, separate tag creation #16098
Conversation
cben
changed the title
|
@miq-bot add-label enhancement, providers/containers |
cben
changed the title
cben
force-pushed
the
tag-mapper
branch
2 times, most recently
from
bb6c481 to
6e5d485
Compare
cben
changed the title
|
@moolitayer @zgalor @djberg96 The 3 PRs are ready now, please review. I might have to untangle them to actually get them merged, but that would just add then remove some transient ugliness, let's review the actual changes first. |
The Mapper will exist during mapping, and finding/creating tags, but not necessary for actual [un]assigning them. The Mapper keeps track of all mapped tags. This commit requires simultaneous changes in providers (kubernetes, amazon) to use new API and pass a mapper to save_inventory.
| class ContainerLabelTagMapping | ||
| # Performs most of the work of ContainerLabelTagMapping - holds current mappings, | ||
| # computes applicable tags, and creates/finds Tag records - except actually [un]assigning. | ||
| class Mapper |
There was a problem hiding this comment.
@cben could you add YARD doc format, at least for the public methods? Can be theoretically done in another PR, but a new class is a nice opportunity. :-)
There was a problem hiding this comment.
Sure 👍 Learning it now...
| @@ -45,6 +45,8 @@ def save_ems_cloud_inventory(ems, hashes, target = nil, disconnect = true) | |||
| _log.debug("#{log_header} hashes:\n#{YAML.dump(hashes)}") | |||
| end | |||
|
|
|||
| hashes[:tag_mapper].find_or_create_tags if hashes[:tag_mapper] | |||
There was a problem hiding this comment.
Is the if condition ok? Seems like this would never run for Cloud?
There was a problem hiding this comment.
I see, that is set in AWS repo. So jut a question, why is if hashes[:tag_mapper] here and not in SaveInventoryContainer
There was a problem hiding this comment.
IINM it will be set by ManageIQ/manageiq-providers-amazon#316 (but not other cloud providers, hence condition).
But I just run amazon specs assuming they prove tag mapping works — if as you say they're commented out, then I didn't really test it...
(To clarify, at this stage I only expect it to work for old save_inventory path.)
There was a problem hiding this comment.
ok, make sense, my brain must be still rebooting after the weekend :-)
|
LGTM 👍 |
|
added docs, PTAL |
|
Checked commits cben/manageiq@480f865~...a2185b1 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 app/models/container_label_tag_mapping/mapper.rb
|
|
Fixed 1 rubocop in docs (deliberately rejecting the @agrare These are ready, are you able to merge in all 3 repos? |
|
@cben yes I can merge in all three, let me take a final look at them all |
|
Weird, github isn't picking up the merge commit b0d6bb8 |
ContainerLabelTagMapping::Mapper class, separate tag creation
|
@cben Can we backport this to Fine/Gaprindashvili? |
ContainerLabelTagMapping::Mapper class, separate tag creation
ContainerLabelTagMapping::Mapper class, separate tag creation
|
This is already in gaprindashvili. |
|
BTW, if you only need classic refresh mapping in Fine, another approach is skip this PR and modify backported refresh parsers to use old interface — like ManageIQ/manageiq-providers-amazon#316 in reverse. |
Overview/plan issue: ManageIQ/manageiq-providers-kubernetes#126
To be merged simultaneously with ManageIQ/manageiq-providers-kubernetes#130 & ManageIQ/manageiq-providers-amazon#316.
A refactoring and first step towards tag mapping with graph refresh.
https://bugzilla.redhat.com/show_bug.cgi?id=1470021
The functional change besides extracting code to a Mapper class is making Mapper responsible for keeping track of mapped tags, and a batch API to find/create all of them.
Currently it has just array and calls find_or_create_tag for each, but in future PR that will turn into an InventoryCollection (with some custom logic) that will be usable as part of graph refresh.
retag_entitykept outside Mapper class because it may be used later / in separate process(soon in post refresh, possibly queued; later collector/persistor split might also require this).