Azure labeling and tagging support #3697
Conversation
There was a problem hiding this comment.
👍 on UI. Attaching screenshot you sent on gitter:
[UPDATE: SCREENSHOT NO LONGER MATCHES PR EXACTLY]
before:
after:
(one translation is missing, dictionary.model.ManageIQ::Providers::Azure::CloudManager::Image)
See comment on needed backend change to support these options.
| # "Vm" => "Amazon", | ||
| # "Image" => "Amazon", | ||
| 'Vm' => [nil, 'Amazon', 'Azure'].freeze, | ||
| 'Image' => [nil, 'Amazon', 'Azure'].freeze, |
There was a problem hiding this comment.
The key of this hash — e.g. 'Image' — is still what's saved as the ContainerLabelTagMapping labeled_resource_type, for each of these options?
When the mapping is performed during refresh, mapper won't be able to distinguish whether user wanted it to apply to only amazon images, azure images or both.
=> You do need distinct labeled_resource_type values for each option.
And you'll need to extend ContainerLabelTagMapping::Mapper to apply mappings with several values. Currently it matches mappings with given type, and nil:
https://github.com/ManageIQ/manageiq/blob/84aae96783c3/app/models/container_label_tag_mapping/mapper.rb#L62-L66
you'll need to match 3 values.
AlexanderZagaynov
force-pushed
the
BZ-1493041
branch
3 times, most recently
from
2c7345d
to
73b5729
Compare
AlexanderZagaynov
force-pushed
the
BZ-1493041
branch
from
73b5729
to
d4f0437
Compare
AlexanderZagaynov
changed the title
|
Checked commit AlexanderZagaynov@d4f0437 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
Hi @martinpovolny can you review this please? |
|
Looks good, but "pending core", right? |
|
@martinpovolny yeah, it should be merged after this one: ManageIQ/manageiq-providers-azure#231 |
| @@ -79,7 +82,7 @@ def entity_ui_name_or_all(entity) | |||
| if entity | |||
| provider = MAPPABLE_ENTITIES[entity] | |||
| model = case entity | |||
| when 'Vm' | |||
| when 'Vm', 'VmAzure' | |||
There was a problem hiding this comment.
The problem is that this break the pluggable providers approach. But it seems to be the case before, so if @martinpovolny is ok with this, I guess we can merge.
We should refactor this though next, so this generic code doesn't know anything about k8s, AWS or Azure
There was a problem hiding this comment.
I am "ok" with that given this is a blocker BZ.
Code like this should not exist in the UI. The UI should never test for "azure" or anything like that. We should only use the supports? interface from the UI.
I am going to merge this PR, but with the expectation that there's going to be a follow up that fixes this and this too:
provider.azure? && entity.sub(/Azure$/, '...
There was a problem hiding this comment.
See proposed refactor followup: 22a532f
(might also help reviewing this PR — the explicit values there were computed by actually running logic from entity_ui_name_or_all and label_tag_mapping_add in this PR).
It still won't be "pluggable", there is currently no mechanism by which providers report which types can be mapped in their refresh (and UI shouldn't offer types that wouldn't work).
I'm happy to later do a bigger followup with a migration to a better schema, but I'm afraid the answer at this point will be "don't bother", justifiably so...
|
@martinpovolny dependent PR merged. |
Azure labeling and tagging support (cherry picked from commit 9945ede) https://bugzilla.redhat.com/show_bug.cgi?id=1568807
|
Gaprindashvili backport details: |
|
@miq-bot add_label fine/yes |
Azure labeling and tagging support (cherry picked from commit 9945ede) https://bugzilla.redhat.com/show_bug.cgi?id=1493041
|
Fine backport details: |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1568806
related: