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

Extract factories to provider gems #11413

Merged
merged 2 commits into from
Sep 23, 2016

Conversation

durandom
Copy link
Member

@durandom durandom commented Sep 21, 2016

Provider specific factories should be located in the provider repository.
Since the manageiq codebase still uses provider specific factories a lot, we include all factories from providers. This can be undone once we are clean.

once #11083 is merged we can use Vmdb::Plugins instead of looking at Rails::Engine

Depends on ManageIQ/manageiq-providers-amazon#48
Unfortunately this is a 🐔 🥚 PR - Without this being merged amazon will fail with Factory already registered and without the amazon PR being merged, this one will fail with Unknown Factory :floating_ip_amazon. I suggest merging the amazon PR first.

That this is only the first PR in making the specs more loosely coupled from the providers 😄

@miq-bot add_labels test, refactoring, pluggable providers
@miq-bot assign chrisarcand

@jrafanie @chrisarcand pls review

@durandom
Copy link
Member Author

see #11414 for moving forward on this one

@chrisarcand
Copy link
Member

@durandom One thought: Is it wise to effectively couple Factories that are used in the tests of MIQ to the release of a provider gem should changes need to be required? Initially that seems unwise to me and perhaps the tests related to the provider in MIQ should be deleted or moved to the gem first.

However, I also acknowledge that you shouldn't need to change the factory for a given spec in MIQ and can do whatever is necessary (albeit ugly) to not have to change the gem.

In other words: Just making sure you've considered that. Sounds like you have quite the plan in action on this and it's going in a great direction.

@durandom
Copy link
Member Author

Is it wise to effectively couple Factories that are used in the tests of MIQ

This is what I'm trying to remove.
So, the use of provider specific factories are all over the specs.

  1. move provider specific factories to provider
  2. convert specs that make use of them to shared examples like in convert floating_ip spec to shared example #11414
  3. test the specific provider model with that shared example inside the provider gem

once the above is completed for everything, we can remove the inclusion of provider factories from the miq codebase. And 1. and 2. should ideally go into one PR, but thats not really possible for some widespread factories.

Sounds like you have quite the plan in action

not really 😄 just making small steps and see what happens. If you have a better idea, I'm all ears

@durandom
Copy link
Member Author

Oh, and most of the specifics to a factory is setting the :class. I am thinking of providing that factory by default via a helper

@chrisarcand
Copy link
Member

This looks fine to me 👍

Whoever can merge this, merge the associated provider PR first, then rerun specs on this one, then merge this one if green. This is to avoid causing a red MIQ master at all (far worse than the amazon provider specs failing for a bit).

@miq-bot
Copy link
Member

miq-bot commented Sep 22, 2016

Checked commits durandom/manageiq@f2ec3be~...fd9d623 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍪

@durandom
Copy link
Member Author

@miq-bot assign @jrafanie

@jrafanie can you merge this? @chrisarcand gave his 👍

@miq-bot miq-bot assigned jrafanie and unassigned chrisarcand Sep 22, 2016
@blomquisg blomquisg merged commit 0aa3437 into ManageIQ:master Sep 23, 2016
@blomquisg blomquisg added this to the Sprint 47 Ending Oct 3, 2016 milestone Sep 23, 2016
@durandom durandom deleted the extract_factories branch September 23, 2016 18:04
@chessbyte
Copy link
Member

@durandom @blomquisg This did not make it into Euwe branch. Please label euwe/yes or euwe/no, as appropriate.

@durandom
Copy link
Member Author

durandom commented Sep 24, 2016

@chessbyte thanks for the heads up

@miq-bot add_label euwe/yes

chessbyte pushed a commit that referenced this pull request Sep 29, 2016
Extract factories to provider gems
(cherry picked from commit 0aa3437)
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit 598289daaa9e75d57cc22ce9270182739883036d
Author: Greg Blomquist <[email protected]>
Date:   Fri Sep 23 13:24:24 2016 -0400

    Merge pull request #11413 from durandom/extract_factories

    Extract factories to provider gems
    (cherry picked from commit 0aa343726958977803f3c314380551cf11a24918)

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.

6 participants