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

New Microsoft Azure docker-machine driver #3159

Merged
merged 7 commits into from Mar 15, 2016
Merged

Conversation

@ahmetb
Copy link
Contributor

ahmetb commented Mar 7, 2016

The new driver uses Azure Resource Manager APIs and offers a lot
more functionality compared to the old Azure driver. It is also
easier to authenticate and does not require user to create and place
certificate files. It only has a single required argument.

This is a breaking change: The new driver cannot work with machines
created with the older Azure driver and vice versa (as the APIs are
entirely different and resources are not shared between old/new azure
APIs).

The new driver addresses many issues about the azure driver reported
so far. Please read azure.md for docs about what's new.

This resolves #2742, resolves #1368, resolves #1142, resolves #2236,
resolves #2408, resolves #1126, resolves #774.

// NOTE(ahmetalpbalkan): any driver initialization I do here gets lost
// afterwards, especially for non-Create RPC calls. Therefore I am mostly
// making rest of the driver stateless by just relying on the following
// piece of info.

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

Which pieces of info are you missing here? On operations subsequent to create invocation, []byte from disk (config.json file) for driver will be unmarshaled into the struct directly, after the NewDriver function returns the initial struct, so it's not unexpected behavior.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@nathanleclaire I believe from what I tried, assigning certain objects during various points of this object's lifetime caused those instances to get lost. So it is not very clear to me at which exact point this config data gets marshaled and unmarshaled.

For instance, when I assign something here during create or pre-create, I saw that it totally gets lost during GetState/GetIP calls that occur later during the machine creation.

For quite a moment I thought maybe it is reinitializing the driver for those calls –but this is not an external driver, I was assuming it'd be in-process and there'd be only a single azure Driver instance throughout the lifetime of a docker-machine execution process; but it appears like due to RPC that's not the case.

I'm still not clear about inner workings of RPC model in docker-machine. I found my way around it but guess I still appreciate any explanation about lifecycle of Driver instance and when/how config gets (un)marshaled.

}

// GetCreateFlags registers the flags this d adds to
// "docker hosts create"

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

Probably best to remote this (probably historical) allusion to docker hosts 😛

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

ohman this is very old. I wish we could leave it here, this might be the last reference to docker hosts, but I'll fix.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

fixed. btw some other drivers have the outdated comment too.

// NOTE(ahmetalpbalkan): We can probably parallelize the sh*t out of this.
// However that would lead to a concurrency logic and while creation of a
// resource fails, other ones would be kicked off, which could lead to a
// resource leak. This is slower but safer.

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

Safest == best for now.

if err != nil {
return err
}
steps := []func() error{

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

What's the reasoning for creating this slice of functions here? Seems to me that they should just be invoked in order, right? If it's just to avoid the if err := f(); err != nil { } song and dance, I'd prefer to actually have that as I feel it's more idiomatic / readable.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

it's just to avoid the if err := f(); err != nil { } song and dance.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

I'll fix if that is more idiomatic or readable to you. This could've looked better with lambdas. (/sigh).

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

fixed this by unrolling funcs into if...;err!=nil{} statements.

if err != nil {
return err
}
for _, f := range []func() error{

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

Same comment here as above. Why?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

fixed this by unrolling funcs into if...;err!=nil{} statements.

if err != nil {
return "", err
}
u := (&url.URL{

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

Seems a bit belabored to make a url.URL and then convert it to string anyway, any reason not to just use fmt.Sprintf directly?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@nathanleclaire hehe we can have a long argument about this. I'm very defensive about repeating the logic of stdlibs. They're there for a reason. For instance, people who have been using : character to split IP:port are essentially going to be broken with IPv6. If should've used net.SplitHostPort, they'd be fine. So I'm basically using the logic in net/url to be on the safe side here. :) It's there for a reason: it helps you to be functionally correct and secure at edge cases you wouldn't even think about.

if err != nil {
return err
}
log.Info("NOTICE: Stopping an Azure Virtual Machine is just going to power it off, not deallocate.")

This comment has been minimized.

Copy link
@nathanleclaire

nathanleclaire Mar 8, 2016

Contributor

So there's a difference between stop and deallocate? Why not make Stop deallocate if that would stop charging? (not saying it's better just wondering)

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@nathanleclaire hmm, deallocating will stop charging. Stopping on the other hand, will keep charging and will keep the IP address the same, which is otherwise released upon deallocation. (for instance if you stop/start google machines, you run into cert issues because IP address is ephemeral; not static, and I took the same approach here). Also starting deallocated VM will be slightly slower to start; starting a stopped VM on the other hand is way faster.

I'm open to any suggestions about what drivers should do in this case.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 9, 2016

Author Contributor

@nathanleclaire what do you think should be the desired behavior here? faster (that costs money and keeps the ephemeral IP address) or slower (that stops costs but changes the ephemeral IP address).

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 8, 2016

Made a few comments / questions but it's looking really good! Thanks for taking the initiative on this.

My general feeling is that I want to err on the side of merging this soon, and testing-as-we-go.

Also, with this branch of development, we're breaking backwards compatibility with old azure-created hosts completely, no? What's our transition plan?

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 8, 2016

@nathanleclaire I have the same opinion, I feel like it's in a usable state already.

Also, with this branch of development, we're breaking backwards compatibility with old azure-created hosts completely, no? What's our transition plan?

It is completely breaking (I have a comment about this in azure.md), there's no way you could even delete Azure machines created with the old driver; and that's because new Azure APIs are completely different and they don’t share any data. There's not much we can do about that other than making the docs better, I believe.

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 8, 2016

Sorry about this gordon noise. Protip: never push this "Update branch" button:

The new driver uses Azure Resource Manager APIs and offers a lot
more functionality compared to the old Azure driver. It is also
easier to authenticate and does not require user to create and place
certificate files. It only has a single required argument.

This is a breaking change: The new driver cannot work with machines
created with the older Azure driver and vice versa (as the APIs are
entirely different and resources are not shared between old/new azure
APIs).

The new driver addresses many issues about the azure driver reported
so far.

This resolves #2742, resolves #1368, resolves #1142, resolves #2236,
resolves #2408, resolves #1126, resolves #774.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@GordonTheTurtle GordonTheTurtle removed the dco/no label Mar 8, 2016
The Azure driver uses the `b39f27a8b8c64d52b05eac6a62ebad85__Ubuntu-15_10-amd64-server-20151116.1-en-us-30GB`
image by default. Note, this image is not available in the Chinese regions. In China you should
specify `b549f4301d0b4295b8e76ceb65df47d4__Ubuntu-15_10-amd64-server-20151116.1-en-us-30GB`.
After authenticating, the driver will remember your credentials up to two weeks.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

It will be good to add a comment saying that, if someone wants to automate using docker-machine, they will have to provide publishsettings or subscriptionID, cert pair

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

Not anymore. Please look at the azure.md as a whole, not a diff. The new driver has nothing to do with the old driver, including the authentication model. It's beneficial to not think of the old Azure driver here at all. The new Azure APIs are a complete paradigm change in pretty much every aspect.

- `--azure-username`: Azure login user name.
Required:

- `--azure-subscription-id`: **(required)** Your Azure Subscription ID.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

No certs? I am a bit confused. My understanding was that subID + cert or publishsettings file has to be provided.

Is the browser based login mandatory now?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala there are no certs or publishSettings files in new Azure APIs. Browser auth is the easiest way for user, the next easiest way is like creating a service principal account in 63 steps, so we went ahead with this.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

How can this be done non-interactively?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala it cannot be done interactively and it is by design. Once you authenticate, it'll credentials remember for 2 weeks. I have discussed this with machine maintainers some time ago and this seems like a more practical solution as it is a lot less tedious for the users compared to the other means of authentication.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Makes sense for interactive users, but we (and a few others like Rancher and DCHQ) are using this as part of our automated workflows to create Docker hosts.

Is there a solution here?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala I was not aware of automated use case of docker-machine by other programs/scripts. I'd need to chat with maintainers about this. It looks like we will go ahead with this model for a while, if it is a supported use case, we may add support for that 63-step authentication model.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 9, 2016

@ahmetalpbalkan we (ContainerX) use it as a building block for provisioning on Azure. We are going support Windows server Docker hosts on Azure and automation is required. I would hate to stay with older version of azure driver for this reason. I can't speak for Rancher, but they posted this last year http://rancher.com/architercutre-of-ranchers-docker-machine-integration/.

I have seen many customers who are using docker-machine in Jenkins/Bamboo workflows. If docker-machine Azure doesn't support automation, where it can be invoked in a script, it's going to be a problem and customers are going to choose different cloud providers that allow easy automation. Also, reentering your credentials every two weeks is just not going work. Consider this scenario, let's say we open a browser window the very first time an IT admin wants to setup Azure as a backend and store the credentials. How are we going to ask them to reenter again? e-mail them to come back and reenter? It's just an ugly workflow.

Can you point me to the documentation for the other authentication model? Is it really 63 steps?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 9, 2016

Author Contributor

@ppadala I agree, as I said, I'll chat with maintainers and implement it. It's not 63 steps but it is some 20 steps, you can search for "azure service principal account", but you won't be able to use it today with this driver. Let's move this discussion to an issue once this PR is merged. I don't see this as a blocking matter for the code review as I've discussed this auth model before with the maintainers.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 10, 2016

Ok, sounds good. I checked azure service principal account and it seems to require AAD. We can discuss this further in a separate issue.

return err
}
if err := c.CreateAvailabilitySetIfNotExists(d.ctx, d.ResourceGroup, d.AvailabilitySet, d.Location); err != nil {
return err

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Don't you have to unwind the resource group creation?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

I am not sure what you mean?

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Line 291: creates a resource group
Line 294: tries to create availabilityset and fails, don't we have to remove the resource group created above?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala I don't think so. Could you please point me to any driver that does that? I believe all drivers work like this by design.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 10, 2016

This seems to be a bad design in docker-machine overall. Leaving the cleanup of dangling cloud objects to users seems bad. It's up to you whether you want to add this to Azure driver (we have made changes to vSphere driver (to be submitted upstream) to take care of some of the cleanup problems).

If there's a way to delete all these objects in one shot with docker-machine rm -f , that would be fine instead of unwinding for every failure, which can become complicated and error prone.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 10, 2016

Author Contributor

@ppadala yes, an interrupted create can be cleaned up perfectly with rm in this new azure driver.

This comment has been minimized.

Copy link
@ppadala
return err
}
if err := c.CreateNetworkSecurityGroup(d.ctx, d.ResourceGroup, d.naming().NSG(), d.Location, d.ctx.FirewallRules); err != nil {
return err

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Same here. Don't you have to remove the objects created earlier?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

like what?

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

See above comment. I am trying to understand, if the objects created earlier need to be removed in the case of failures.

if err != nil {
return err
}
if err := c.DeleteVirtualMachineIfExists(d.ResourceGroup, d.naming().VM()); err != nil {

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Is it better to do this at the end of removing other objects?

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala No, I don't think so. We create two types of resources:

  1. Resources who are attached to VM: (e.g. Network interface, IP address, Firewall) you can't delete those without deleting the VM or detaching those resources first. Deleting VM just disassociates these resources so that Delete calls below work just fine.
  2. Resources that hold the VM: (e.g. Resource Group, Availability Set, Virtual Network, Subnet) You can't remove those without removing the VM first, it's like deleting a non-empty folder. The API does not allow it.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Ok, thanks for the clarification.

sshPort = 22
)

// Driver represents Azure Docker Machine Driver.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

Can you add a comment for information for Environment, ResourceGroup etc.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala umm not sure why I should comment each field. I haven't seen any other driver doing it and they're sort of self-documenting through (a) naming (b) documentation.

This comment has been minimized.

Copy link
@ppadala

ppadala Mar 8, 2016

No need to comment each field, just link at the top of the structure or driver file, that points to relevant Azure APIs. Note that, for people outside Microsoft, there's good amount of learning, cross-referencing with go-sdk etc.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Mar 8, 2016

Author Contributor

@ppadala all these Azure concepts can be googled very easily and they are not cryptic (so that only msft employees would understand). I don't see the point of adding a ton of "how Azure works" comments here. godoc might not be the right place for an Azure lecture. :)

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 9, 2016

Is there any way to provide users a script or something that will migrate old versions of VMs created with the azure driver to this new driver? I'm starting to feel that I was a bit hasty with the merge removing the old driver and considering reverting it. Our deprecation policy is two releases so dropping support for a driver so abruptly has me concerned.

Let's talk about options to preserve and migrate existing azure VMs, since I'm sure at least some are out there. Maybe we can rename and include the existing driver as azure-legacy and bump config versions to do a migration of existing azure VMs (at config version 3) to have the driver name azure-legacy. Then over time, we will phase it out completely.

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 9, 2016

@nathanleclaire fair point. I was not aware of two versions policy, though I remember asking about it in our earlier conversations (:P).

A migration is just not possible. What I can offer is, maybe we keep azure driver around for another release and call this `azure-new then fix it later?

Basically if you ask me, I don't think people will be disappointed if we break with a nice error message saying "hey this version doesn't support old Azure VMs, please go back to v0.6.0", but up to you.

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 9, 2016

Or we can call this azure and when an old VM is triggered, we can offer a link to migration script that modifies existing VMs to azure-old. Feel free to pick one of these solutions @nathanleclaire, I will happily implement.

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 9, 2016

So, consensus on our end after talking with @dmp42 is that:

  1. We are fine with moving forward using the new Azure driver, however-
  2. Any existing azure machines machines should error out when any operation with the next released version of Machine (after merging this code in) is attempted. A "deprecated" type warning suggesting the user switch to the new model in order to use the new version of Machine should be shown.
  3. Likewise, no destructive updates to the config.json should be made. It must be possible for the user to use Machine 0.6.0 to tie up loose ends (e.g. docker-machine rm) with the legacy driver before moving to the new one.
  4. We will also put a notice about this in the release notes.

How does that sound? Are you willing to work with us on verifying that the above works as expected? I've not had much success with the legacy Azure driver in the past unfortunately. The tricky bit in the above will be preventing damage to the config.json -- it gets loaded/saved in quite a few places.

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 9, 2016

@nathanleclaire That sounds good. It looks like I just have to make sure we don't damage config.json and terminate early when we detect the old azure driver (and tell users about deprecation and downgrading to rm old machines).

Actually this new driver already errors out and doesn't damage config.json for old Azure VMs but I will make the error clearer.

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 9, 2016

Actually this new driver already errors out and doesn't damage config.json for old Azure VMs but I will make the error clearer.

OK sounds great. Thanks Ahmet.

@dgageot

This comment has been minimized.

Copy link
Contributor

dgageot commented Mar 13, 2016

LGTM

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 14, 2016

@dgageot thanks for reviewing and testing, I really appreciate we found that bug before the release. We currently don't know how many users are getting this different uppercase/lowercase output from the APIs but we got it covered for now.

@dgageot

This comment has been minimized.

Copy link
Contributor

dgageot commented Mar 14, 2016

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 14, 2016

Going to take it for a test drive, then most likely merge. cc @londoncalling for docs review; if there are any problems, I suggest we open and issue and do a follow-up PR.


Grab your subscription ID from the portal, then run `docker-machine create` with these details:
First time you try to create a machine, Azure driver will ask you to

This comment has been minimized.

Copy link
@londoncalling

londoncalling Mar 15, 2016

Contributor

The first time you try to ...etc.


You need to create a subscription with a cert. Run these commands and answer the questions:
> **NOTE:** This documentation is for the new version of Azure driver started

This comment has been minimized.

Copy link
@londoncalling

londoncalling Mar 15, 2016

Contributor

This documentation is for the new version of the Azure driver, which started shipping with v0.7.0. This driver is not backwards-compatible with the old Azure driver.

You need to create a subscription with a cert. Run these commands and answer the questions:
> **NOTE:** This documentation is for the new version of Azure driver started
> shipping with v0.7.0 and it is not backwards-compatible with the old Azure
> driver. If you like to manage your existing Azure machines, please download

This comment has been minimized.

Copy link
@londoncalling

londoncalling Mar 15, 2016

Contributor

If you want to continue managing your existing Azure machines, please download ...

@londoncalling

This comment has been minimized.

Copy link
Contributor

londoncalling commented Mar 15, 2016

I made some wording corrections in inline comments for azure.md, but haven't had time to test the driver yet. If you like, I can update for flow of the text in the azure.md file after this is merged. There's just a lot of nitpicky things I would change throughout about the wording.

@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Mar 15, 2016

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "azure-arm" git@github.com:ahmetalpbalkan/machine.git somewhere
$ cd somewhere
$ git rebase -i HEAD~7
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Ammending updates the existing PR. You DO NOT need to open a new one.

@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Mar 15, 2016

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "azure-arm" git@github.com:ahmetalpbalkan/machine.git somewhere
$ cd somewhere
$ git rebase -i HEAD~7
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Ammending updates the existing PR. You DO NOT need to open a new one.

1 similar comment
@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Mar 15, 2016

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "azure-arm" git@github.com:ahmetalpbalkan/machine.git somewhere
$ cd somewhere
$ git rebase -i HEAD~7
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Ammending updates the existing PR. You DO NOT need to open a new one.

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 15, 2016

@londoncalling thanks for reviewing, just addressed docs comments in a separate commit (9fd035e)

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 15, 2016

@ahmetalpbalkan How exactly does one get an Azure subscription ID? This screen doesn't have any and can't figure out how to get one.

screen shot 2016-03-14 at 5 35 57 pm

@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 15, 2016

@nathanleclaire hmm that's weird I didn't know you could have an Azure account without having a subscription. Try http://azure.com/free to get a Trial Subscription.

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 15, 2016

Guess I had a free trial one that expired so maybe Azure got confused. I am starting the whole funnel again from that link you sent me -- think this should work.

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

nathanleclaire commented Mar 15, 2016

Create and other expected operations works smoothly for me (and create is miles ahead of the old driver in terms of user experience -- excellent job!).

LGTM

Thanks and congratulations on all your good work Ahmet and everyone else involved in bringing this to fruition.

nathanleclaire added a commit that referenced this pull request Mar 15, 2016
New Microsoft Azure docker-machine driver
@nathanleclaire nathanleclaire merged commit ef4823f into docker:master Mar 15, 2016
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docker/dco-signed All commits signed
Details
documentation success
Details
@ahmetb

This comment has been minimized.

Copy link
Contributor Author

ahmetb commented Mar 15, 2016

@nathanleclaire @dgageot It's very delightful to hear all these, thank you for taking time to review this as well. I will be sending the improvements incrementally.

@ppadala

This comment has been minimized.

Copy link

ppadala commented Mar 16, 2016

Great work @ahmetalpbalkan! I have started rebasing my change. Also, filed an issue based on our discussion above about automation.

@londoncalling

This comment has been minimized.

Copy link
Contributor

londoncalling commented on 9fd035e Mar 21, 2016

@ahmetalpbalkan LGTM! sorry for late reply but I think this is already merged anyway. Looks nice.

@edevil

This comment has been minimized.

Copy link

edevil commented Mar 25, 2016

Great work!

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.