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

Extend InventoryCollectionDefault::NetworkManager with network_groups #16136

Merged
merged 1 commit into from
Oct 11, 2017

Conversation

miha-plesko
Copy link
Contributor

With this commit we implement a class function that is needed when implementing graph inventory refresh for NetworkManager that support NetworkGroup inventory object. The new network_groups function provides hash of common properties that are needed by persister.

@Ladas I've assigned you since I remember you being in charge of introducing graph refresh. Please let me know if someone else should be assigned instead.

@miq-bot add_label enhancement
@miq-bot assign @Ladas

/cc @gberginc

With this commit we implement a class function that is needed when
implementing graph inventory refresh for NetworkManager that supports
NetworkGroup inventory object. The new network_groups function provides
hash of common properties that are needed by persister.

Signed-off-by: Miha Pleško <[email protected]>
@miq-bot
Copy link
Member

miq-bot commented Oct 6, 2017

Checked commit miha-plesko@254c9d7 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

For the NetworkGroup itself, I am still not sure whether it gets things clearer or not. It acts the same as CloudNetwork now. So maybe we should keep using CloudNetwork model, until the behavior will become different?

@Ladas
Copy link
Contributor

Ladas commented Oct 6, 2017

@miq-bot assign @blomquisg

@miq-bot miq-bot assigned blomquisg and unassigned Ladas Oct 6, 2017
@miha-plesko
Copy link
Contributor Author

I'm not sure what the rationale to use NetworkGroup was either, but I think Nuage guys are using miq in production already (not 100% sure about this). I don't think I have balls to go change this without consulting them first 😄

My plan would be to implement graph inventory refresh that works exactly like their current legacy refresher and then drop the legacy refresher completely. Then, having only one fansy refresher, I'd go into changing models if requested. Sounds good?

@miha-plesko
Copy link
Contributor Author

@blomquisg I think Ladas and myself converged to the point where only merge button needs to be pressed. Naturally, if you have any comments, I'd be glad to hear them.

@blomquisg blomquisg assigned agrare and unassigned blomquisg Oct 11, 2017
@blomquisg
Copy link
Member

Punting over to @agrare because I think he's been following more of the general networking discussions than I have.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

This looks good for parity with current refresh
If we want to merge NetworkGroup and CloudNetwork that will need a migration to move existing data. If this is something you want to do we should do it soon before schema freeze (a few weeks).

@agrare agrare added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 11, 2017
@agrare agrare merged commit 254c9d7 into ManageIQ:master Oct 11, 2017
agrare added a commit that referenced this pull request Oct 11, 2017
Extend InventoryCollectionDefault::NetworkManager with network_groups
@miha-plesko miha-plesko deleted the network_groups branch January 7, 2019 08:22
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.

5 participants