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

[catalog_controller_spec.rb] Fix ems let... again #7508

Conversation

NickLaMuro
Copy link
Member

This is a follow up to 02e4b85 (aka: #7500 ) 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.

Links

@NickLaMuro
Copy link
Member Author

@miq-bot add_label test

@himdel If I could have you review this one when you have a chance, that would be great. Thanks!

@NickLaMuro
Copy link
Member Author

Seems to be a bunch of failures across manageiq-ui-classic at the moment, so going to wait until that is resolved to rebase the again.

That said, the current job at the time of writing:

https://github.com/ManageIQ/manageiq-ui-classic/runs/1397568112

Is only failing on jest specs, so clearly there is nothing failing from this code (as far as I can tell).

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 NickLaMuro force-pushed the rails6_fix_has_many_through_factory_errors_part_deux branch from ee6461a to 16b4501 Compare November 16, 2020 16:27
@miq-bot
Copy link
Member

miq-bot commented Nov 16, 2020

Checked commit NickLaMuro@16b4501 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
1 file checked, 1 offense detected

spec/controllers/catalog_controller_spec.rb

  • ⚠️ - Line 747, Col 13 - Rails/ActiveRecordAliases - Use update instead of update_attributes.

@NickLaMuro
Copy link
Member Author

@himdel and/or @h-kataria :

Could one of you take a look at this now that specs (on master) are passing again?

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.
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.

Lgtm

@himdel himdel merged commit dc44613 into ManageIQ:master Nov 18, 2020
@himdel himdel self-assigned this Nov 18, 2020
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