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

use :automation_manager_ansible_tower instead of :automation_manager #586

Merged
merged 1 commit into from
Oct 7, 2019

Conversation

jameswnl
Copy link
Contributor

Corresponding to the refactoring of factories in ManageIQ/manageiq#19330

@jameswnl
Copy link
Contributor Author

@jrafanie @d-m-u check this out

@d-m-u
Copy link
Contributor

d-m-u commented Sep 27, 2019

I don't think this is the right way to do this. Fiiiiine. But also, LJ's right. Please see the work done in ManageIQ/manageiq#18842 for https://bugzilla.redhat.com/show_bug.cgi?id=1671844. The factory shouldn't be an intermediate class.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3652

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.6%) to 96.795%

Totals Coverage Status
Change from base Build 3644: 5.6%
Covered Lines: 2748
Relevant Lines: 2839

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3652

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+5.6%) to 96.795%

Totals Coverage Status
Change from base Build 3644: 5.6%
Covered Lines: 2748
Relevant Lines: 2839

💛 - Coveralls

@miq-bot
Copy link
Member

miq-bot commented Sep 27, 2019

Checked commit jameswnl@8887d60 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

@d-m-u what do you think? These might have been using an automation manager but the tests are clearly using an ansible tower automation manager. I think this makes sense to use the provider names here since that's what being tested.

@d-m-u
Copy link
Contributor

d-m-u commented Sep 28, 2019

Then how are we addressing the current issue with the validation in the main repo? I'm fine with this if that gets addressed as well, but it feels problematic to have a single intermediate factory.

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

If the test are about tower - then the test should point to a tower factory

@d-m-u
Copy link
Contributor

d-m-u commented Sep 28, 2019

@jrafanie
Copy link
Member

I don't think this is the right way to do this. Please see the work done in ManageIQ/manageiq#18842 for https://bugzilla.redhat.com/show_bug.cgi?id=1671844. The factory shouldn't be an intermediate class.

I think we agree. For this specific set of tests, they seem to be testing tower automation manager so the change in this PR looks right the right thing to do.

We'll need to fix the base factory as referenced above in a different PR as it's not usable right now.

@gmcculloug gmcculloug merged commit 4de4f86 into ManageIQ:master Oct 7, 2019
@gmcculloug gmcculloug self-assigned this Oct 7, 2019
@gmcculloug gmcculloug added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 7, 2019
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