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

Moving some of redhat/infra_manager code to separate model #11482

Merged

Conversation

borod108
Copy link

@borod108 borod108 commented Sep 23, 2016

This is just a small step in the direction of making this more object oriented, for now are least this cosmetic change is done to help work on another PR.

@borod108
Copy link
Author

@miq-bot add_label "refactoring"
@miq_bot assign @durandom

@miq-bot
Copy link
Member

miq-bot commented Sep 23, 2016

@borod108 Cannot apply the following label because they are not recognized: "refactoring"

@durandom
Copy link
Member

@miq-bot add_label refactoring, providers, providers/rhevm

@borod108 you are missing the include_concern "ApiIntergration"

@durandom
Copy link
Member

@miq-bot assign durandom

@chessbyte chessbyte changed the title Moving some of redhat/infra_manager code to seperate model Moving some of redhat/infra_manager code to separate model Sep 23, 2016
@borod108 borod108 force-pushed the ref/split_api_integration_from_infra_manager branch from 96ab800 to 473bd48 Compare September 23, 2016 10:21
@borod108
Copy link
Author

@durandom oh thank you for catching this, I added that line and the did some more changes and removed it by mistake...

@borod108 borod108 force-pushed the ref/split_api_integration_from_infra_manager branch from 473bd48 to fae2360 Compare September 23, 2016 11:09
@borod108
Copy link
Author

borod108 commented Sep 23, 2016

@durandom can we merge this please? all the rubocop are for old code.

@borod108 borod108 closed this Sep 23, 2016
@borod108 borod108 reopened this Sep 23, 2016
@Fryguy
Copy link
Member

Fryguy commented Sep 23, 2016

@durandom Are you good with this?

@durandom
Copy link
Member

@Fryguy yeah, once CI is 🍏 I'm good :shipit:

And I trust @borod108 that he just copied the methods - did not check line by line :)

@Fryguy
Copy link
Member

Fryguy commented Sep 23, 2016

Tests are 🔴 😕

@durandom Is this a pattern we should do in the other providers to keep them consistent (i.e. create an ApiIntegration concern)?

@durandom
Copy link
Member

Is this a pattern we should do in the other providers to keep them consistent (i.e. create an ApiIntegration concern)?

No, in the end it's a decision to be made the providers' author. In this case it makes sense, because they do quite some Api introspection. Whereas, say for amazon, the api integration is just the connect method.

We made this, in order to make the follow up PR focus only on the changing part not the moving of methods part.

ps. I'd be great to have all the Manager subclasses only implement the methods required by its base class. So, moving this to a concern and maybe later to its own class sounds like the right direction to me.

@durandom
Copy link
Member

@borod108 does the test fail also locally for you?

This is just a small step in the direction of making this
more object oriented, for now are least this cosmetic change
is done to help work on another PR.
@borod108 borod108 force-pushed the ref/split_api_integration_from_infra_manager branch from fae2360 to 2bc6c56 Compare September 25, 2016 09:36
@miq-bot
Copy link
Member

miq-bot commented Sep 25, 2016

Checked commit borod108@2bc6c56 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 14 offenses detected

app/models/manageiq/providers/redhat/infra_manager/api_integration.rb

@borod108
Copy link
Author

@durandom no it was passing locally, anyway I fetched and rebased on master and now its green.

@durandom
Copy link
Member

@Fryguy it's green. Can you merge in case you have no objections?

@Fryguy Fryguy merged commit 7988378 into ManageIQ:master Sep 26, 2016
@Fryguy Fryguy added this to the Sprint 47 Ending Oct 3, 2016 milestone Sep 26, 2016
chessbyte pushed a commit that referenced this pull request Sep 29, 2016
…m_infra_manager

Moving some of redhat/infra_manager code to separate model
(cherry picked from commit 7988378)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit fd81918d7640995d5d79d43655c058f72a8b9e8b
Author: Jason Frey <[email protected]>
Date:   Mon Sep 26 16:22:20 2016 -0400

    Merge pull request #11482 from borod108/ref/split_api_integration_from_infra_manager

    Moving some of redhat/infra_manager code to separate model
    (cherry picked from commit 7988378a22971254d7e5eea1a8070280801dd957)

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