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

[rails6][service_template_ansible_playbook_spec.rb] Fix ems/provider lets #20787

Merged

Conversation

NickLaMuro
Copy link
Member

This fix takes a page from a PR from @skateman:

#20031

Which was that instead of trying to assign the provider prior to making the EMS in the factories, default a provider object type as part of the ext_management_system child definition, and reference it in the let(:provider).

This is not a problem in Rails 5.2, but does lead to the following error in Rails 6.0

5) ServiceTemplateAnsiblePlaybook.create_catalog_item creates and returns a catalog item

   Failure/Error: factory_exists?(factory) ? super : class_from_symbol(factory).create!(*args)

   ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection:

     Cannot modify association 'ManageIQ::Providers::EmbeddedAnsible::Provider#endpoints' because the source reflection class 'Endpoint' is associated to 'ExtManagementSystem' via :has_many.

   # ./spec/support/missing_factory_helper.rb:10:in `create'
   # ./spec/models/service_template_ansible_playbook_spec.rb:11:in `block (2 levels) in <top (required)>'
   # ./spec/models/service_template_ansible_playbook_spec.rb:7:in `block (2 levels) in <top (required)>'
   # ./spec/models/service_template_ansible_playbook_spec.rb:15:in `block (2 levels) in <top (required)>'
   # ./spec/models/service_template_ansible_playbook_spec.rb:40:in `block (2 levels) in <top (required)>'
   # ./spec/models/service_template_ansible_playbook_spec.rb:47:in `block (2 levels) in <top (required)>'
   # ./spec/models/service_template_ansible_playbook_spec.rb:82:in `block (3 levels) in <top (required)>'

Most likely do to a fix that I was unable to track down, but the following git log command is most likely to contain some of the commits with the fix:

$ git log --oneline -E --grep "has.*many.*through" 5-2-stable..6-0-stable

Links

@NickLaMuro
Copy link
Member Author

@miq-bot assign @jrafanie

Hopefully this works pre-rails6 so it is one less commit that needs to be included in the rails-6 branch. Currently this is on that branch, but I plan to remove it if it can be merged into master.

@NickLaMuro
Copy link
Member Author

@miq-bot add_label rails6

@miq-bot miq-bot added the rails6 label Nov 6, 2020
@NickLaMuro NickLaMuro mentioned this pull request Nov 6, 2020
39 tasks
…lets

This fix takes a page from a PR from David:

ManageIQ#20031

Which was that instead of trying to assign the provider prior to making
the EMS in the factories, default a provider object type as part of the
`ext_management_system` child definition, and reference it in the
`let(:provider)`.

This is not a problem in Rails 5.2, but does lead to the following error
in Rails 6.0

  5) ServiceTemplateAnsiblePlaybook.create_catalog_item creates and returns a catalog item

     Failure/Error: factory_exists?(factory) ? super : class_from_symbol(factory).create!(*args)

     ActiveRecord::HasManyThroughCantAssociateThroughHasOneOrManyReflection:

       Cannot modify association 'ManageIQ::Providers::EmbeddedAnsible::Provider#endpoints' because the source reflection class 'Endpoint' is associated to 'ExtManagementSystem' via :has_many.

     # ./spec/support/missing_factory_helper.rb:10:in `create'
     # ./spec/models/service_template_ansible_playbook_spec.rb:11:in `block (2 levels) in <top (required)>'
     # ./spec/models/service_template_ansible_playbook_spec.rb:7:in `block (2 levels) in <top (required)>'
     # ./spec/models/service_template_ansible_playbook_spec.rb:15:in `block (2 levels) in <top (required)>'
     # ./spec/models/service_template_ansible_playbook_spec.rb:40:in `block (2 levels) in <top (required)>'
     # ./spec/models/service_template_ansible_playbook_spec.rb:47:in `block (2 levels) in <top (required)>'
     # ./spec/models/service_template_ansible_playbook_spec.rb:82:in `block (3 levels) in <top (required)>'

Most likely do to a fix that I was unable to track down, but the
following `git log command` is most likely to contain some of the
commits with the fix:

  $ git log --oneline -E --grep "has.*many.*through" 5-2-stable..6-0-stable
@NickLaMuro NickLaMuro force-pushed the fix_service_template_ansible_playbook_spec branch from c4a124d to 79c2826 Compare November 10, 2020 16:31
@miq-bot
Copy link
Member

miq-bot commented Nov 10, 2020

Checked commit NickLaMuro@79c2826 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@agrare
Copy link
Member

agrare commented Nov 12, 2020

This seems to have caused manageiq-content to start failing: https://travis-ci.com/github/ManageIQ/manageiq-content/jobs/432734386#L1996-L2003

@NickLaMuro
Copy link
Member Author

@agrare thanks for the heads up. I will take a look at it later today

@NickLaMuro
Copy link
Member Author

So just a bit of a update:

Seems like doing:

FactoryBot.create(:embedded_ansible_automation_manager)

Creates 2 AutomationManager records... which is weird. Might be a seeding thing we did with the Provider class, so it might something where we should instead remove this factory and only create the Provider records. Will find more proof of this theory before I suggest what to do next.

@NickLaMuro
Copy link
Member Author

Okay, yes, it looks like the AutomationManager is being created twice with this since the ensure_managers class is fired on a before_validation:

31a2f32

I think the solution for this is to deprecate creating :embedded_ansible_automation_manager records directly (change the provider :factory => ... line to a raise, and then change all callers of this to ensure that we create providers directly, and have them reference them through the provider directly (inverse of what I ended up doing in this commit).

I will try that out and see how that works.

NickLaMuro added a commit to NickLaMuro/manageiq-ui-classic that referenced this pull request Nov 13, 2020
This is a follow up to 02e4b85 as the solution provided there, while
works for this use case, ended up causing problems in
`ManageIQ/manageiq-content`:

ManageIQ/manageiq#20787 (comment)

Instead, we use the :provider_embedded_ansible factory, and reference
the `.automation_manager` from that.  In addition, since a update to the
`:inventory_root_groups` was needed, we also modify the record in a
`before` statement as well.

This ends up not being as clean as the previous implementation, but
makes sure we are using the `EmbeddedAnsible` provider in the form that
it would be called when used in the application.
NickLaMuro added a commit to NickLaMuro/manageiq-ui-classic that referenced this pull request Nov 13, 2020
This is a follow up to 02e4b85 as the solution provided there, while
works for this use case, ended up causing problems in
`ManageIQ/manageiq-content`:

ManageIQ/manageiq#20787 (comment)

Instead, we use the :provider_embedded_ansible factory, and reference
the `.automation_manager` from that.  In addition, since a update to the
`:inventory_root_groups` was needed, we also modify the record in a
`before` statement as well.

This ends up not being as clean as the previous implementation, but
makes sure we are using the `EmbeddedAnsible` provider in the form that
it would be called when used in the application.
NickLaMuro added a commit to NickLaMuro/manageiq-ui-classic that referenced this pull request Nov 13, 2020
This is a follow up to 02e4b85 as the solution provided there, while
works for this use case, ended up causing problems in
`ManageIQ/manageiq-content`:

ManageIQ/manageiq#20787 (comment)

Instead, we use the :provider_embedded_ansible factory, and reference
the `.automation_manager` from that.  In addition, since a update to the
`:inventory_root_groups` was needed, we also modify the record in a
`before` statement as well.

This ends up not being as clean as the previous implementation, but
makes sure we are using the `EmbeddedAnsible` provider in the form that
it would be called when used in the application.
NickLaMuro added a commit to NickLaMuro/manageiq-ui-classic that referenced this pull request Nov 16, 2020
This is a follow up to 02e4b85 as the solution provided there, while
works for this use case, ended up causing problems in
`ManageIQ/manageiq-content`:

ManageIQ/manageiq#20787 (comment)

Instead, we use the :provider_embedded_ansible factory, and reference
the `.automation_manager` from that.  In addition, since a update to the
`:inventory_root_groups` was needed, we also modify the record in a
`before` statement as well.

This ends up not being as clean as the previous implementation, but
makes sure we are using the `EmbeddedAnsible` provider in the form that
it would be called when used in the application.
NickLaMuro added a commit to NickLaMuro/manageiq-ui-classic that referenced this pull request Nov 16, 2020
Merge ManageIQ#7508 instead!

This is a follow up to 02e4b85 as the solution provided there, while
works for this use case, ended up causing problems in
`ManageIQ/manageiq-content`:

ManageIQ/manageiq#20787 (comment)

Instead, we use the :provider_embedded_ansible factory, and reference
the `.automation_manager` from that.  In addition, since a update to the
`:inventory_root_groups` was needed, we also modify the record in a
`before` statement as well.

This ends up not being as clean as the previous implementation, but
makes sure we are using the `EmbeddedAnsible` provider in the form that
it would be called when used in the application.
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