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.

Sign up for GitHub

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

Jump to bottom

Change get_targets_for_source to use correct params #16811

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Change get_targets_for_source to use correct params #16811

merged 1 commit into from
Jan 12, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 11, 2018 •
edited by gmcculloug
Loading

Source needs to be src and the last param in this call needs to be the correct association name.

Previously, cloud network list was only updated after selection of security group, or change of tab, or a select set of other actions that completely voided the list of parameters passed to the filtering method. This issue was fixed in #16688. Unfortunately I also introduced an issue where get_targets_for_source was not returning the right things because I wasn't calling it correctly, which this PR should fix.

Related to:

ManageIQ/manageiq-content#241
https://bugzilla.redhat.com/show_bug.cgi?id=1518847

@d-m-u d-m-u mentioned this pull request Jan 11, 2018
Merged
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

@miq-bot assign @gmcculloug

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

@miq-bot add_label provisioning

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

@syncrou

@d-m-u d-m-u changed the title Changes get_targets_for_source to use correct params [WIP] Changes get_targets_for_source to use correct params Jan 11, 2018
@miq-bot miq-bot added the wip label Jan 11, 2018
@d-m-u d-m-u force-pushed the fixing_allowed_cloud_network_method branch from caae6b8 to 6467ee1 Compare January 12, 2018 15:22
@d-m-u d-m-u changed the title [WIP] Changes get_targets_for_source to use correct params Changes get_targets_for_source to use correct params Jan 12, 2018
@miq-bot miq-bot removed the wip label Jan 12, 2018
syncrou
syncrou approved these changes Jan 12, 2018
View reviewed changes
Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@d-m-u d-m-u changed the title Changes get_targets_for_source to use correct params Change get_targets_for_source to use correct params Jan 12, 2018
@d-m-u d-m-u force-pushed the fixing_allowed_cloud_network_method branch from 6467ee1 to 59ba714 Compare January 12, 2018 16:27
@miq-bot
Copy link
Member

miq-bot commented Jan 12, 2018

Checked commit d-m-u@59ba714 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

gmcculloug
gmcculloug reviewed Jan 12, 2018
View reviewed changes
app/models/manageiq/providers/cloud_manager/provision_workflow.rb
@@ -95,7 +95,6 @@ def availability_zone_to_cloud_network(src)
hash[cn.id] = cn.name
end
else
return {} unless load_ar_obj(src[:ems]).cloud_subnets
Copy link
Member

Choose a reason for hiding this comment

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

Nice, glad you were able to remove this check. 👍

@gmcculloug gmcculloug merged commit d9b5cb5 into ManageIQ:master Jan 12, 2018
@gmcculloug gmcculloug added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 12, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 12, 2018

@miq-bot add_label gaprindashvili/yes

@d-m-u d-m-u deleted the fixing_allowed_cloud_network_method branch January 12, 2018 17:16
simaishi pushed a commit that referenced this pull request Jan 12, 2018
@gmcculloug @simaishi
Merge pull request #16811 from d-m-u/fixing_allowed_cloud_network_method
2350723 
Change get_targets_for_source to use correct params
(cherry picked from commit d9b5cb5)

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

Gaprindashvili backport details:

$ git log -1
commit 235072348a3b05e8258783f9ed8cb59e7c388a5c
Author: Greg McCullough <[email protected]>
Date:   Fri Jan 12 12:10:45 2018 -0500

    Merge pull request #16811 from d-m-u/fixing_allowed_cloud_network_method
    
    Change get_targets_for_source to use correct params
    (cherry picked from commit d9b5cb535570821a3af093cd626b6889102b6e76)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1533277

This was referenced Jan 15, 2018
Merged
Closed
Merged
This was referenced Jan 17, 2018
Merged
Merged
Merged
Merged
@simaishi
Copy link
Contributor

@gmcculloug @d-m-u Since this PR was backported to gaprindashvili, don't we want to also backport the spec file change PRs that are linked in this PR?

@gmcculloug
Copy link
Member

@simaishi Yes I would agree with that. There is also a non-spec PR listed above: ManageIQ/manageiq-providers-openstack#197

The remaining ones all appear to be spec only. @d-m-u Are there any other PRs related to this work that may not be listed here?

@gmcculloug
Copy link
Member

Ignore that first part, I found that PR ManageIQ/manageiq-providers-openstack#197 was replaced by ManageIQ/manageiq-providers-openstack#202 which is already back-ported.

@simaishi
Copy link
Contributor

@d-m-u Can you please add gaprindashvili/yes labels to the appropriate PRs? It seems there are more than what's linked in this PR. For example.. this PR has a link to #16824 (this fixes what this PR broke?), which says ManageIQ/manageiq-providers-amazon#394 is a related test change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug approved these changes

@syncrou syncrou syncrou approved these changes

Assignees

@gmcculloug gmcculloug

Labels
gaprindashvili/backported lifecycle/provisioning
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@d-m-u @miq-bot @simaishi @gmcculloug @syncrou