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

Security Group provisioning backend #11758

Merged

Conversation

@gildub
Copy link
Contributor

gildub commented Oct 7, 2016

Controllers/Models: EMS Network Provider using task queue
Will integrate with following UI PR including tests

@tzumainn

This comment has been minimized.

Copy link
Contributor

tzumainn commented Oct 7, 2016

@miq-bot add_label euwe/yes

@miq-bot miq-bot added the euwe/yes label Oct 7, 2016
@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch Oct 10, 2016
@tzumainn

This comment has been minimized.

Copy link
Contributor

tzumainn commented Oct 10, 2016

The create/delete actions look good! Update doesn't seem to actually update the sec group name though... ?

@gildub

This comment has been minimized.

Copy link
Contributor Author

gildub commented Oct 11, 2016

@tzumainn,
Thanks for the review. To update security group, fog-openstack 0.1.15 is effectively required,
meanwhile since 0.1.14 blocks the test validation - as we discussed with @rwsu, due to baremetal-introspection change, #11747.

@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch Oct 13, 2016
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Oct 17, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@tzumainn

This comment has been minimized.

Copy link
Contributor

tzumainn commented Oct 17, 2016

fog-openstack 0.1.15 was merged, os the functionality look good to me now!

@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch 8 times, most recently Oct 18, 2016
@gildub gildub changed the title UI: Adds security group provisioning UI: Security Group provisioning (Network Provider) Oct 18, 2016
@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch Oct 18, 2016
@gildub

This comment has been minimized.

Copy link
Contributor Author

gildub commented Oct 18, 2016

@dclarizio , @himdel , @martinpovolny

This is a refreshed version using Task Queue, could you please review?

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Oct 18, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch 4 times, most recently Oct 18, 2016
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Oct 19, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch 2 times, most recently Oct 19, 2016
@blomquisg blomquisg closed this Oct 21, 2016
@blomquisg blomquisg reopened this Oct 21, 2016
@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch Oct 22, 2016
@gildub

This comment has been minimized.

Copy link
Contributor Author

gildub commented Oct 22, 2016

UI label to be removed please.

@chessbyte , @tzumainn

@tzumainn

This comment has been minimized.

Copy link
Contributor

tzumainn commented Oct 22, 2016

@miq-bot remove_label ui

@miq-bot miq-bot removed the ui label Oct 22, 2016
@@ -111,4 +112,25 @@ def create_network_router_queue(userid, options = {})
}
MiqTask.generic_action_with_callback(task_opts, queue_opts)
end

def create_security_group(options)
SecurityGroup.raw_create_security_group(self, options)

This comment has been minimized.

Copy link
@Ladas

Ladas Oct 24, 2016

Contributor

should not be raw method

This comment has been minimized.

Copy link
@gildub

gildub Oct 24, 2016

Author Contributor

@Ladas, the called method is wrapping the raw API call and therefore ought to bear raw in the its name, isn't?

SecurityGroup.raw_create_security_group(self, options)
end

def create_security_group_queue(userid, options = {})

This comment has been minimized.

Copy link
@Ladas

Ladas Oct 24, 2016

Contributor

duplicated code with create_network_router_queue

This comment has been minimized.

Copy link
@gildub

gildub Oct 24, 2016

Author Contributor

@Ladas, yes I thought about going DRY here too, meanwhile I believe this is out of scope for this PR. Could I suggest another pass to tackle this across the other wait_for_queue that have made it so far?

This comment has been minimized.

Copy link
@Ladas

Ladas Oct 25, 2016

Contributor

@gildub yeah, we will have to do more refactoring though, all the models needs to be consistent (base class no raw methods, raw methods and supports without suffix )

app/models/manageiq/providers/openstack/network_manager/security_group.rb Outdated
}
queue_opts = {
:class_name => self.class.name,
:method_name => 'delete_security_group',

This comment has been minimized.

Copy link
@Ladas

Ladas Oct 24, 2016

Contributor

where is this method defined? Same for others

This comment has been minimized.

Copy link
@gildub

gildub Oct 25, 2016

Author Contributor

@Ladas, sorry, yes, delete_security_group and update_security_group, should be both prefixed with raw because they're actually calling the raw API commands. Fixed it.

@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch Oct 25, 2016
@tzumainn

This comment has been minimized.

Copy link
Contributor

tzumainn commented Oct 27, 2016

@Ladas Does this look okay to you now, other than some greater refactoring for post-Euwe?

@Ladas

This comment has been minimized.

Copy link
Contributor

Ladas commented Oct 27, 2016

@tzumainn sure, lets put it in and do refactoring not backported to euwe

@Ladas

This comment has been minimized.

Copy link
Contributor

Ladas commented Oct 27, 2016

@miq-bot miq-bot assigned blomquisg and unassigned dclarizio and mzazrivec Oct 27, 2016
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Oct 27, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch 2 times, most recently Oct 28, 2016
Models: EMS Network Provider using task queue
UI to follow and integrate with tests
@gildub gildub force-pushed the gildub:network-provisioning-security-groups branch to e949f53 Oct 31, 2016
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Oct 31, 2016

Checked commit gildub@e949f53 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
4 files checked, 0 offenses detected
Everything looks good. ⭐️

@blomquisg blomquisg merged commit b4916f9 into ManageIQ:master Nov 1, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.008%) to 36.874%
Details
chessbyte added a commit that referenced this pull request Nov 1, 2016
…roups

Security Group provisioning backend
(cherry picked from commit b4916f9)
@chessbyte

This comment has been minimized.

Copy link
Member

chessbyte commented Nov 1, 2016

Euwe Backport details:

$ git log -1
commit 7f32628f6da9432f108769fabe3890c910a37fb2
Author: Greg Blomquist <blomquisg@gmail.com>
Date:   Tue Nov 1 09:52:04 2016 -0400

    Merge pull request #11758 from gildub/network-provisioning-security-groups

    Security Group provisioning backend
    (cherry picked from commit b4916f93c8be94c724e98a83b23a36a4fef5adf6)
@chessbyte chessbyte added euwe/backported and removed euwe/yes labels Nov 1, 2016
@gildub gildub deleted the gildub:network-provisioning-security-groups branch Nov 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.