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

[Fine] Fine tag mapping #17068

Closed
wants to merge 4 commits into from
Closed

Conversation

juliancheal
Copy link
Member

Backport of Tag mapping in Graph refresh

#16734
plus additional methods required

Fixes BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1537111

@juliancheal juliancheal changed the title Fine tag mapping [Fine] [WIP] Fine tag mapping Feb 28, 2018
@miq-bot miq-bot changed the title [Fine] [WIP] Fine tag mapping [WIP] [Fine] [WIP] Fine tag mapping Feb 28, 2018
@miq-bot miq-bot added the wip label Feb 28, 2018
@juliancheal juliancheal changed the title [WIP] [Fine] [WIP] Fine tag mapping [WIP] [Fine] Fine tag mapping Feb 28, 2018
@juliancheal
Copy link
Member Author

Needed for ManageIQ/manageiq-providers-amazon#417

@juliancheal juliancheal changed the title [WIP] [Fine] Fine tag mapping [Fine] Fine tag mapping Mar 2, 2018
@miq-bot miq-bot removed the wip label Mar 2, 2018
@juliancheal
Copy link
Member Author

@cben does this look correct to you?

@cben
Copy link
Contributor

cben commented Mar 14, 2018

I'll take a deeper look tomorrow, but I suspect some more stuff needs to be backported. E.g. controlled_by_mapping seems not used anywhere, ContainerLabelTagMapping::Mapper doesn't exist yet, so I think this still can't map in graph refresh (?)

See ManageIQ/manageiq-providers-kubernetes#126 for cheat sheet of PRs (wow, that's horrifically long 😳)

@juliancheal
Copy link
Member Author

I suspect some more stuff needs to be backported. E.g. controlled_by_mapping seems not used anywhere, ContainerLabelTagMapping::Mapper doesn't exist yet, so I think this still can't map in graph refresh (?)

@cben will explain why I wasn't sure if it was working ManageIQ/manageiq-providers-amazon#417 (comment)

I'll compare more against that PR thanks!

@bronaghs
Copy link

@cben -
@ AlexanderZagaynov is implementing tagging support for the Azure provider, it is a blocker for the next Fine errata. Is his work dependent on this PR getting merged?

FYI his PRs are:
#17212
ManageIQ/manageiq-providers-azure#231

@cben
Copy link
Contributor

cben commented Apr 11, 2018

Did Azure provider support graph refresh in Fine?
IIUC you're separating the graph refresh from classic refresh PRs in Azure tag mapping precisely because you only need classic refresh in Fine?

Hmm, on second thought there is some dependency there.
I changed the interface of the ContainerLabelTagMapping when I added graph refresh support. So just taking a refresh parser using the new interface and backporting it will not work against the old interface.
I think the minimal parts you need to backport to Fine are:

Then if you'll also want mapping in graph refresh in Fine (for Amazon?), you'd need a much bigger part of ManageIQ/manageiq-providers-kubernetes#126... I'm not sure that's worth it & safe enough to backport.

@juliancheal
Copy link
Member Author

Thanks @cben

@miq-bot
Copy link
Member

miq-bot commented Apr 13, 2018

Checked commits juliancheal/manageiq@8808666~...6ad243b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 3 offenses detected

app/models/manageiq/providers/cloud_manager.rb

@juliancheal
Copy link
Member Author

Decided not to go ahead with backporting tag mapping to Fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants