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

Modify label_tag_mapping_add to accommodate scoped entity type. #666

Merged
merged 3 commits into from
Mar 16, 2017
Merged

Modify label_tag_mapping_add to accommodate scoped entity type. #666

merged 3 commits into from
Mar 16, 2017

Conversation

djberg96
Copy link
Contributor

This PR is based on the changes in ManageIQ/manageiq#14288. In that PR the mappable types are scoped by provider, e.g. "Amazon::Vm" or "Kubernetes::ContainerNode".

Instead of automatically using the hard coded AUTOTAG_PREFIX, it parses the provider name from the selected item if possible. The AUTOTAG_PREFIX is still the default value, which preserves backwards compatibility.

Copy link
Contributor

@cben cben left a comment

Choose a reason for hiding this comment

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

cc @zgalor See also core PR ManageIQ/manageiq#14288 for context.

prefix, entity = entity.split('::')
# Backwards compatibility
if prefix && entity
prefix.downcase!
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider just having lowercase prefixes in MAPPABLE_ENTITIES.
Capitalized with :: looks like provider class name, but it's not exactly, IMO more confusing then helpful.

But then you might need upcase to display as "Amazon/Vm" in UI? BTW, will we want translation of Amazon.
BTW, your screenshot shows "Amazon/Vm" with slash, where does :: -> / replacement happen?

else
prefix = ContainerLabelTagMapping::AUTOTAG_PREFIX
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This method was already too long, please extract a method to compute category name (or some other split).

@cben
Copy link
Contributor

cben commented Mar 14, 2017

4 ems_container_controller_spec, container_node_controller_spec failures unrelated, fixed on ui-classic master.
2 label_tag_mapping_spec failures relevant of course.

@djberg96
Copy link
Contributor Author

@cben, Yeah, the UI depends on the MAPPABLE_ENTITIES, so I didn't want to downcase those. I'm not sure where the "/" to "::" conversion happens right off. I remember seeing it somewhere buried in the code, but it's definitely what I saw in the logs.

I've refactored the method a bit and added some custom specs. I'm not sure what Rubocop is whining about exactly.

@djberg96 djberg96 changed the title [WIP] Modify label_tag_mapping_add to accomdate scoped entity type. Modify label_tag_mapping_add to accomdate scoped entity type. Mar 15, 2017
@djberg96
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Mar 15, 2017
@djberg96
Copy link
Contributor Author

@blomquisg Ok, ready to go.

@blomquisg
Copy link
Member

ManageIQ/manageiq#14288 is merged now.

@blomquisg
Copy link
Member

@miq-bot assign dclarizio

@blomquisg
Copy link
Member

@miq-bot add_label enhancement, cloud/compute

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

@blomquisg Cannot apply the following label because they are not recognized: cloud/compute

@blomquisg
Copy link
Member

@miq-bot add_label compute/cloud

@blomquisg
Copy link
Member

@miq-bot remove_label "pending core"

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

@blomquisg Cannot remove the following label because they are not recognized: "pending core"

@blomquisg
Copy link
Member

@miq-bot remove_label pending core

@djberg96
Copy link
Contributor Author

@dclarizio @AparnaKarve Look alright? Can we merge?

@AparnaKarve
Copy link
Contributor

@djberg96 Tested successfully in the UI.

Other than the PR #, LGTM :)

@dclarizio
Copy link

@AparnaKarve I see "container" on a lot of those screens, can you comment on what else needs to be changed? Thx, Dan

@AparnaKarve
Copy link
Contributor

I see "container" on a lot of those screens, can you comment on what else needs to be changed?

@dclarizio Good point.

@djberg96 So looks like this is not applicable to Containers alone anymore, the entity could also be Amazon now.

To keep it more generic we could just lose the Containers word in the haml.
More like --
Choose an entity and label in the first screenshot, and
Entity and Label in the table in the second screenshot

screenshot#1
screen shot 2017-03-16 at 11 28 24 am

screenshot#2
screen shot 2017-03-16 at 11 28 12 am

@djberg96
Copy link
Contributor Author

djberg96 commented Mar 16, 2017

@AparnaKarve @dclarizio ~~~Yes, those should be reworked but I would like to save it for a future PR. In the next iteration, the plan is to add another selection field (provider). Can that wait until then?~~~ Nevermind, I guess it's just a few text updates, I'll update the PR.

@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2017

Checked commits https://github.com/djberg96/manageiq-ui-classic/compare/8bc11ee9b973f723350f3fb527a00855faa97bea~...c298c141381f15d2dd5c4185d33a00b17ee9e5f3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 10 offenses detected

app/controllers/ops_controller/settings/label_tag_mapping.rb

spec/controllers/ops_controller/settings/label_tag_mapping_spec.rb

@dclarizio dclarizio changed the title Modify label_tag_mapping_add to accomdate scoped entity type. Modify label_tag_mapping_add to accommodate scoped entity type. Mar 16, 2017
@dclarizio dclarizio merged commit ce62aef into ManageIQ:master Mar 16, 2017
@dclarizio dclarizio added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 16, 2017
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.

7 participants