Namespace the mappable object types, add Amazon VM and Image types.

[djberg96]

This adds Amazon images and vm's as a mappable type to the ContainerLabelTagMapping model. Furthermore, I've scoped the names by provider type.

This serves two purposes. First, it modifies the drop down menu, and lets the user know exactly which type of resource that they're mapping:

Second, rather than use the hard coded AUTOTAG_PREFIX constant, the prefix will be parsed from the name. This will require a minor modification on the UI side. The net result will be that you will see entries in this form inside the tags table:

/managed/kubernetes:container_node:container_stuff
/managed/amazon:vm:test_label
/managed/kubernetes::label_for_all
/managed/amazon:image:aws_image_label

The UI modification is here: ManageIQ/manageiq-ui-classic#666

[djberg96]

@blomquisg @cben Please review.

[cben]

See comment I just left on #14236.
I wonder if we really need combinatorial explosion of providers * entities, especially considering that current UI won't let you map 2 entity types to same category.
I'm NOT certain anybody actually needs this on containers side, we implemented it because "the backend already allowed it and it wasn't too hard in UI" ;-) So in case you're only extending it because we already had it, let's talk :-)

And if we do need it, I wonder what resolution of "provider" we need. E.g. "kubernetes" here actually stands for both Kubernetes & Openshift — i.e. any containers provider. And while currently Amazon will be the only other option, will one want separate mappings for Amazon Vms vs Azure Vms vs oVirt Vms?

[djberg96]

@cben, the goal is to automatically map custom attributes from AWS vm's and images with any mappings the user has created. This would then be applied during a refresh. @blomquisg please correct me if I'm wrong.

[cben]

Can user have both vms and images with custom attribute "department=propulsion"? Would they want those mapped to one "Departments" category or independent categories for Vms and Images? I suppose user could even have both amazon vm images and container images labeled with "project=warp-drive". Would they want those mapped to one "Projects" category or independent categories for amazon and containers?

[djberg96]

@cben I will defer to @blomquisg and @bascar for the answer to your questions.

[bascar]

@cben vm and images would use the same tagging, not independent. I think we would need containers and amazon separate since in many cases it is two independent groups who run those environments and they may not want to cooperate on the tagging.

[blomquisg]

So reading through this, I guess we need:

Amazon/All
Amazon/Vm
Amazon/Image
Containers/All
Containers/Project
Containers/...

Instead of one encompassing <ALL> group.

@bascar, @cben, @djberg96 sound about right?

@djberg96 is that signing up for a whole mess of work? Or, is that easy to finesse here?

[djberg96]

@blomquisg Unfortunately that's not so easy because the current code is assuming nil is ALL.

[blomquisg]

Unfortunately that's not so easy because the current code is assuming nil is ALL.

@bascar given the current timeframe for the Fine release, I'm thinking the first phase should be to assume ALL means literally anything that can be mapped from any provider enabled for Tag Mapping.

Currently ALL means anything that can be mapped from any containers provider.

We want to have as little change to the mapping logic as possible to avoid the risk of impacting the Container-specific mappings.

@djberg96 continue under this assumption for now. We can figure out a course correction later if @bascar comments otherwise.

[bascar]

I am good with the minimized impact of the "All" mapping

[cben]

The net result will be that you will see entries in this form inside the tags table:

/managed/kubernetes:container_node:container_stuff
/managed/amazon:vm:test_label
/managed/kubernetes::label_for_all
/managed/amazon:image:aws_image_label

The 3rd one is subtle, it's an "All" mapping, with kubernetes: prefix just for compatibility. There will be no amazon::foo categories for now. I'm OK with that, though at some point kubernetes:: would change meaning from really All to Containers/All (if you do a migration, perhaps we could avoid it).

• Need to change map_labels() calls in kubernetes/container_manager/refresh_parser to use entity_type when with Kubernetes:: prefix.
  • And support no prefix for compat?

I'd prefer to see a migration adding prefix to all existing mappings, rather than compatibility code in several places...

[djberg96]

@cben I agree that we'll need to do a migration that adds a provider column eventually. But, at the moment, there are internal methods depending on that nil to check for ALL.

That will be the next refactoring, but for now I'm keeping it as simple as possible.

[bronaghs]

This is great @djberg96 !

[cben]

I meant a migration changing Foo -> Kubernetes::Foo within a single string column, so you don't need compatibility checks to support both Foo and Kubernetes::Foo.
But if you prefer to have checks first, your call.
👍, a provider column would be even better.

[blomquisg]

I actually had written a message here and forgot to click "Comment", then I saw @cben's comment saying the same thing.

I think there needs to be a data migration to move the existing labeled_resource_type mappings in Container_Label_Tag_Mapping from things like ContainerNode to Kubenetes::ContainerNode.

[djberg96]

@blomquisg At the moment that value isn't touched and just stores "ContainerNode" or "Vm". I'm afraid of what changing that will do. Plus, once we add a provider column to the table, it would become redundant.

[miq-bot]

Checked commits https://github.com/djberg96/manageiq/compare/3231e3993e01b3b8078ec5efada9c9c66b20479c~...9bd7d8d03a15603364079d388f33d429463e5d46 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 5 offenses detected

spec/models/container_label_tag_mapping_spec.rb

• ❗ - Line 38, Col 38 - Style/TrailingWhitespace - Trailing whitespace detected.
• ❗ - Line 42, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 46, Col 35 - Style/TrailingWhitespace - Trailing whitespace detected.
• ❗ - Line 50, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 51, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

[djberg96]

Ignore those last rubocop warnings, they were for a commit that I yanked.

[djberg96]

@miq-bot remove_label wip

[djberg96]

@blomquisg Ok, ready to go.

[blomquisg]

So, the only thing we're looking for as a follow on PR is a provider column in the container_label_tag_mapping. For now, it's fine because it's just two different provider types with mutually exclusive labeled_resource_types.

[cben]

Sorry, had a frantic week.
Wait, Kubernetes::Foo mappings created after this will not be understood by kubernetes refresh.
OK, let me make a fix PR real quick.
I'm going to lift some of your UI compatibility logic into the mapping model, you'll then be able to call it from UI.

[djberg96]

@cben I don't see how. If you select "Kubernetes::ContainerNode", then the tag it generates is "/managed/kubernetes:container_node:foo".

[cben]

See map_labels( calls in container refresh_parser.