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

Add Microsft Azure Virtual Machine Scale Set provider. #12

Merged
merged 1 commit into from
Jan 20, 2018
Merged

Add Microsft Azure Virtual Machine Scale Set provider. #12

merged 1 commit into from
Jan 20, 2018

Conversation

flyinprogrammer
Copy link
Contributor

This allows us to use the ListVirtualMachineScaleSetNetworkInterfaces() API to find the Virtual Machine Scale NICs which are absent in the ListAll() API response.

It will enable us to drastically simplify this work flow for consul server bootstrapping: https://github.com/hashicorp/terraform-azurerm-consul/blob/master/modules/run-consul/run-consul#L109-L129

@flyinprogrammer flyinprogrammer changed the title create azurevmss Add Microsft Azure Virtual Machine Scale Set provider. Sep 21, 2017
@flyinprogrammer
Copy link
Contributor Author

poke

poke 🤷‍♂️

magiconair
magiconair suggested changes Oct 14, 2017
View reviewed changes
Copy link
Contributor

@magiconair magiconair left a comment

Choose a reason for hiding this comment

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

I've left a couple of comments. Can you try to also add some more debug logging since understanding what is going wrong might be difficult in prod. See the aws provider for an example.

discover.go Outdated
@@ -30,6 +31,7 @@ type Provider interface {
var Providers = map[string]Provider{
"aws": &aws.Provider{},
"azure": &azure.Provider{},
"azurevmss": &azurevmss.Provider{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you try to roll this into the existing azure provider since otherwise we're going to duplicate all dependencies. Maybe use a type=vmss parameter to make it unambiguous.

provider/azurevmss/azurevmss_discover.go Outdated
subscription_id: The id of the subscription
secret_access_key: The authentication credential
resource_group_name: The name of the resource group to filter on
virtual_machine_scale_set_name: The name of the virtual machine scale set to filter on
Copy link
Contributor

Choose a reason for hiding this comment

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

These name is too long, IMO. What about vm_scale_set and resource_group?

provider/azurevmss/azurevmss_discover.go Outdated
addrs = append(addrs, *x.PrivateIPAddress)
}
}
return addrs, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

pls add a log.Print("[DEBUG]... like in the other providers.

@magiconair
Copy link
Contributor

Can you also please prefix all commit messages with azure: and lowercase them? Then they're consistent with the rest.

@flyinprogrammer
Copy link
Contributor Author

sweet!!! i can do all these things, I just need time :) I'll get to this asap.

@magiconair
Copy link
Contributor

@flyinprogrammer any update?

@flyinprogrammer
Copy link
Contributor Author

got two weddings this weekend, and next week i'm in london, but hopefully soon? i'll definitely rebase before resubmitting.

@flyinprogrammer
Copy link
Contributor Author 

creating role
sleeping for 30 seconds to let custom role become eventually consistent
creating discover-test-discover user
Built discover.env
Done setting up vmss account.
*** Creating Terraform environment
*** This takes ~ 5min
*** See tf.log for progress

terraform init  >> tf.log 2>&1
terraform get   >> tf.log 2>&1
terraform plan  >> tf.log 2>&1
terraform apply >> tf.log 2>&1
21:29 $ make test
*** Running go-discover test on Azure VMSS
OK

@jbrads
Copy link

jbrads commented Jan 2, 2018

@flyinprogrammer @magiconair I had a need for this functionality just this week, any updates? :)

@flyinprogrammer
Copy link
Contributor Author

@jbrads I believe we're just waiting on a review. @magiconair I'm more than happy to do more edits!

@slackpad slackpad merged commit 4e49190 into hashicorp:master Jan 20, 2018
@slackpad
Copy link
Contributor

LGTM - sorry for the delay on that one!

@slackpad
Copy link
Contributor

hashicorp/consul#3824

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

@magiconair magiconair magiconair requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flyinprogrammer @magiconair @jbrads @slackpad