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

Support multiple network ids and network names #148

Merged
merged 4 commits into from
Apr 13, 2016

Conversation

sue445
Copy link
Contributor

@sue445 sue445 commented Apr 1, 2016

CloudStack network_ids can accept multiple values. But vagrant-cloudstack plugin can not use multiple network_ids (only single network_id).

So I added feature for multiple network ids

Usage

Vagrantfile

Vagrant.configure(2) do |config|
  config.vm.provider :cloudstack do |cloudstack, override|
    # single network id
    cloudstack.network_id = "AAAAA"

    # multiple network ids
    cloudstack.network_id = ["AAA", "BBB"]
  end
end

@@ -126,7 +126,8 @@ to update UUIDs in your Vagrantfile. If both are specified, the id parameter tak
* `instance_ready_timeout` - The number of seconds to wait for the instance
to become "ready" in Cloudstack. Defaults to 120 seconds.
* `domain_id` - Domain id to launch the instance into
* `network_id` - Network uuid that the instance should use
* `network_id` - Network uuid(s) that the instance should use
- `network_id` is single value (e.g. `"AAAA"`) or multiple values (e.g. `["AAAA", "BBBB"]`)
* `network_name` - Network name that the instance should use
Copy link
Contributor

Choose a reason for hiding this comment

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

@sue445 Nice one! But if we do multiple networks by network_id, we should also allow that via network_name

@sue445 sue445 changed the title Support multiple network ids Support multiple network ids and network names Apr 6, 2016
@sue445
Copy link
Contributor Author

sue445 commented Apr 6, 2016

@bheuvel I pushed fixes. Please review again 🙇

@@ -34,8 +34,22 @@ def call(env)

sanitize_domain_config

network_ids =
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you can move this new code block (lines 37-49) to the sanitize_domain_config?

@bheuvel
Copy link
Contributor

bheuvel commented Apr 6, 2016

It's looking good! But I will need to have a closer look later on, so some comments after a quick look:

We may need to support either IDs OR Names and not both at the same time.
This may become a problem is someone would specify IDs id1 id2 and names name3 name2 name1. Due to the sync having a "preference" for IDs, the resulting sync would have id1 id2 id1.

I would suggest to keep it simple and use IDs if present, and only use names if IDs are not present.
So basically @domain_config.network_name = [] unless @domain_config.network_id.nil?
(which could also go into the sanitize_domain_config)
That would also simplify the create_list (and drop the self.?)

@sue445
Copy link
Contributor Author

sue445 commented Apr 11, 2016

I would suggest to keep it simple and use IDs if present, and only use names if IDs are not present.

@bheuvel I fix and push.

@bheuvel
Copy link
Contributor

bheuvel commented Apr 13, 2016

@sue445 All tests passing, did some "manual" testing with multiple network names/ids, and is looking good! Thanks!

@bheuvel bheuvel merged commit f046847 into MissionCriticalCloud:master Apr 13, 2016
@sue445 sue445 deleted the multiple_network_id branch April 14, 2016 02:56
@sue445
Copy link
Contributor Author

sue445 commented Apr 14, 2016

😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants