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

Return empty array for delegation with nil manager #19473

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Nov 6, 2019

If a NetworkManager doesn't have a parent_manager (e.g. Nuage) return an empty array instead of a nil

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1767747

Alternative to #19459 by @agrare

@kbrock kbrock force-pushed the bz_1767747_fix_network_manager_delegate_without_parent_manager branch from a216be8 to 4975f8d Compare November 6, 2019 19:09
@kbrock kbrock added the bug label Nov 6, 2019
@kbrock kbrock force-pushed the bz_1767747_fix_network_manager_delegate_without_parent_manager branch from 4975f8d to 7c695f7 Compare November 6, 2019 19:11
If a NetworkManager doesn't have a parent_manager (e.g. Nuage) return an empty array instead of a nil

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1767747
@kbrock kbrock force-pushed the bz_1767747_fix_network_manager_delegate_without_parent_manager branch from 7c695f7 to c475153 Compare November 6, 2019 19:49
@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2019

Checked commit kbrock@c475153 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 19 offenses detected

app/models/manageiq/providers/network_manager.rb

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 looks good

@agrare agrare self-assigned this Nov 7, 2019
agrare added a commit that referenced this pull request Nov 7, 2019
…delegate_without_parent_manager

Return empty array for delegation with nil manager
@agrare agrare merged commit c475153 into ManageIQ:master Nov 7, 2019
@kbrock kbrock deleted the bz_1767747_fix_network_manager_delegate_without_parent_manager branch November 7, 2019 15:34
@mzazrivec
Copy link
Contributor

Rails console after this change:

irb(main):002:0> MiqExpression.miq_adv_search_lists('CloudNetwork', :exp_available_finds)
PostgreSQLAdapter#log_after_checkout, connection_pool: size: 30, connections: 1, in use: 1, waiting_in_queue: 0
Traceback (most recent call last):
       16: from railties (5.1.7) lib/rails/commands/console/console_command.rb:97:in `perform'
       15: from railties (5.1.7) lib/rails/commands/console/console_command.rb:17:in `start'
       14: from railties (5.1.7) lib/rails/commands/console/console_command.rb:62:in `start'
       13: from (irb):2
       12: from lib/miq_expression.rb:903:in `miq_adv_search_lists'
       11: from lib/miq_expression.rb:802:in `model_details'
       10: from lib/miq_expression.rb:891:in `get_relats'
        9: from lib/miq_expression.rb:942:in `build_relats'
        8: from lib/miq_expression.rb:942:in `each'
        7: from lib/miq_expression.rb:973:in `block in build_relats'
        6: from lib/miq_expression.rb:942:in `build_relats'
        5: from lib/miq_expression.rb:942:in `each'
        4: from lib/miq_expression.rb:952:in `block in build_relats'
        3: from activerecord (5.1.7) lib/active_record/reflection.rb:404:in `klass'
        2: from activerecord (5.1.7) lib/active_record/reflection.rb:408:in `compute_class'
        1: from activerecord (5.1.7) lib/active_record/inheritance.rb:166:in `compute_type'
NameError (uninitialized constant ManageIQ::Providers::NetworkManager::AuthKeyPair)

And the following CI failures in ui-classis: https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/608753209?utm_medium=notification&utm_source=github_status

@martinpovolny
Copy link
Member

martinpovolny commented Nov 8, 2019

I think that this PR nicely shows that the idea of running the tests on the other repos (UI) only on demand is not that great.

@lpichler
Copy link
Contributor

lpichler commented Nov 8, 2019

@kbrock

  1. for :key_pairs maybe you wanted to use something likes this:
has_many :key_pairs, -> { where.not(:resource_id => nil) }, :class_name => "Authentication", :as => :resource
  1. moving relations from :delegate to many :has_many puts this relations to the reflections.
    so this method starts to return reflections_with_virtual them for example in MiqExpression.build_relats here and this is source of error.

kbrock added a commit to kbrock/manageiq that referenced this pull request Nov 8, 2019
While we would like to avoid delegation, easier to just point to
the parent manager for auth key pairs

fixes but with ManageIQ#19473
@agrare agrare added this to the Sprint 124 Ending Nov 11, 2019 milestone Nov 26, 2019
@simaishi
Copy link
Contributor

@kbrock Can this be ivanchuk/yes?

@kbrock
Copy link
Member Author

kbrock commented Dec 18, 2019

Note: if you merge this, you also need #19486 which undoes one of the changes

simaishi pushed a commit that referenced this pull request Jan 7, 2020
…delegate_without_parent_manager

Return empty array for delegation with nil manager

(cherry picked from commit fe3f8ea)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1785374
@simaishi
Copy link
Contributor

simaishi commented Jan 7, 2020

Ivanchuk backport details:

$ git log -1
commit a195152a4b57fd56956c774295972489db6d3abd
Author: Adam Grare <[email protected]>
Date:   Thu Nov 7 08:51:57 2019 -0500

    Merge pull request #19473 from kbrock/bz_1767747_fix_network_manager_delegate_without_parent_manager

    Return empty array for delegation with nil manager

    (cherry picked from commit fe3f8ea91a23a88195aa9879913a5b0180b30e56)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1785374

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