Issue 10732045: Implement more methods in the Azure provider.
|
Created:
11 years, 9 months ago by rvb Modified:
11 years, 9 months ago Reviewers:
jtv.canonical, mp+171966 Visibility:
Public. |
DescriptionImplement more methods in the Azure provider.
In the Azure provider, this branch implements environ's Instances() and AllInstances() methods and instance's Id(), DNSName() and WaitDNSName() methods. Nothing particularly noteworthy about the implementation: all the methods are implemented by doing simple calls to gwacl's methods. The tests are made relatively simple by the usage of the new test utility methods that gwacl provides. You'll see that I have decided, in tests, to only check that _a_ request was made to the fake Azure server rather that checking the exact nature of the request. The gwacl methods are themselves properly unittested so I decided it would do more bad than good to put too much Azure-specific stuff (such as the requested url) in the provider's test code.
https://code.launchpad.net/~rvb/juju-core/az-list-instances/+merge/171966
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 15
MessagesTotal messages: 10
Please take a look.
Sign in to reply to this message.
not LGTM — by and large I like this branch a lot, but there are some hazards to future maintenance that I think need addressing first. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go File environs/azure/environ.go (right): https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:111: if len(ids) == 0 { Document what you're doing here, because it's a pitfall for future maintenance. The code suggests that this is about a distinction between nil and an empty slice, but in reality it's about a nasty edge case in the two different behaviours of instances(). https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:123: // instances is an internal method which returns the instances matching the The lower-case name already says that it's internal. No need to repeat that in the comment, I think. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:124: // given instance ids or all the instances if 'ids' is empty. These are two very different things to do, with a very nasty invitation for failure between them — an empty array can mean either "I want all" or "I want none." Don't casually introduce something like that as an afterthought! I think there's a better way to do this; see below. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:125: // If the some of the intances could not be found, it returns the instance Three typos in this line: "If the some of the intances," and at the end of the line, "the instance" should be "the instances." Maybe even "those instances" to make it clearer. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:126: // that could be found plus the error environs.ErrPartialInstances in the error Phrasing is a bit awkward... Maybe just break up the sentence? Something like "in that case the error return will be environs.ErrPartialInstances." https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:129: // Acquire management API object. In other languages, these blocks of code preceded by comments are considered a "code smell" indicative of a need to extract smaller functions. But in this case I find them really helpful — a problem with Go and its error returns is that extracting functions doesn't really help make the code shorter or easier to read. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:138: deploymentNames := make([]string, len(ids)) The preparation of deploymentNames is specific to just one of the two call sites. Why not pass deploymentNames as the parameter, and leave the work that's specific to Instances() inside Instances()? https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:145: deployments, err := context.ListDeployments(request) Don't forget to check that error return! https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... environs/azure/environ.go:154: if len(ids) != 0 && len(ids) != len(instances) { I guess the check for len(ids) here distinguishes between the two different callers. That's basically the Swiss Army Knife antipattern: the function does two different things that just happen to share a lot of code, and a parameter selects between the two behaviours. The fact that the parameter also provides other information doesn't help. Why not move this check up into Instances()? AFAICT AllInstances() doesn't need this part of the dance anyway, and instances() will be a bit easier to understand. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go File environs/azure/environ_test.go (right): https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:101: var propertiesS1 = gwacl.HostedService{ Why is this a global variable? AFAICT it should be local to patchWithPropertiesResponse (more about that below). The name would also be less mystifying if this weren't a package-global variable. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:126: c.Check(len(instances), Equals, 2) Please don't do this! The 2 is entirely arbitrary here, and relates to a global variable that this test doesn't even mention. That's just building a timebomb into your test. Maybe it's not so bad if the global is a single, well-known constant, but here the connections go through the innards of a compound global. Code isn't just instructions for the machine, it's also an explanation for humans. So say what you mean, not just what you want the computer to do: always use the appropriate len() expression for a thing like this. In the Unix tradition, reviewers would put it as: "you misspelled len(propertiesS1.Deployments)." If you want to keep the test maintainable, why not make the list of deployment names a parameter to patchWithPropertiesResponse(), and build the HostedService object there? Implicitly sharing data between the test and the factory through a global variable is really bad practice. https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go... environs/azure/environ_test.go:133: instances, err := env.Instances([]instance.Id{"deployment-1"}) Here, too, you're implicitly referring to a global variable elsewhere that isn't mentioned. It may seem like a nitpick now, while the test is (relatively) small and still fresh in your mind and nobody else has modified it. But think kindly of the engineer who is under pressure to fix a bug and isn't very familiar with the code, because at some point it might be you. https://codereview.appspot.com/10732045/diff/1/environs/azure/instance.go File environs/azure/instance.go (right): https://codereview.appspot.com/10732045/diff/1/environs/azure/instance.go#new... environs/azure/instance.go:30: // An Azure deployment gets his DNS name when it's created. Not "his" but "its"! Don't get into the habit of confusing "its" and "it's" though, or I shall have to unfriend you on Friendface. :-) https://codereview.appspot.com/10732045/diff/1/environs/azure/instance_test.go File environs/azure/instance_test.go (right): https://codereview.appspot.com/10732045/diff/1/environs/azure/instance_test.g... environs/azure/instance_test.go:18: // Deployment's hostnames are randomly generated by Azure. If you mean hostnames of deployments (plural), then the apostrophe comes after the s: "Deployments' hostnames." Don't we get to select our own hostnames? I thought we did. Maybe I'm confusing hostnames and deployment names... https://codereview.appspot.com/10732045/diff/1/environs/azure/instance_test.g... environs/azure/instance_test.go:28: c.Check(azInstance.Id(), Equals, instance.Id(deploymentName)) As in the environ name: don't weave implicit data relationships between factories and tests through globals!
Sign in to reply to this message.
On 2013/06/28 14:54:20, jtv.canonical wrote: > not LGTM — by and large I like this branch a lot, but there are some hazards to > future maintenance that I think need addressing first. > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go > File environs/azure/environ.go (right): > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:111: if len(ids) == 0 { > > Document what you're doing here, because it's a pitfall for future maintenance. > The code suggests that this is about a distinction between nil and an empty > slice, but in reality it's about a nasty edge case in the two different > behaviours of instances(). Done. > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:123: // instances is an internal method which returns > the instances matching the > > The lower-case name already says that it's internal. No need to repeat that in > the comment, I think. Not sure it matters tbh. > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:124: // given instance ids or all the instances if > 'ids' is empty. > > These are two very different things to do, with a very nasty invitation for > failure between them — an empty array can mean either "I want all" or "I want > none." Don't casually introduce something like that as an afterthought! I > think there's a better way to do this; see below. As discussed (at length) I'd like to keep all the gwacl-related logic and objects inside method. I think it's much more easy to reason about what the method is doing that way. If gwacl provider a ListAllDeployments, this would probably be a bit clearer. > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:125: // If the some of the intances could not be > found, it returns the instance > > Three typos in this line: "If the some of the intances," and at the end of the > line, "the instance" should be "the instances." Maybe even "those instances" to > make it clearer. Fixed. > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:126: // that could be found plus the error > environs.ErrPartialInstances in the error > > Phrasing is a bit awkward... Maybe just break up the sentence? Something like > "in that case the error return will be environs.ErrPartialInstances." > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:138: deploymentNames := make([]string, len(ids)) > > The preparation of deploymentNames is specific to just one of the two call > sites. Why not pass deploymentNames as the parameter, and leave the work that's > specific to Instances() inside Instances()? > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:145: deployments, err := > context.ListDeployments(request) > > Don't forget to check that error return! Oops, done. > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ.go#newc... > environs/azure/environ.go:154: if len(ids) != 0 && len(ids) != len(instances) { > > I guess the check for len(ids) here distinguishes between the two different > callers. That's basically the Swiss Army Knife antipattern: the function does > two different things that just happen to share a lot of code, and a parameter > selects between the two behaviours. The fact that the parameter also provides > other information doesn't help. > > Why not move this check up into Instances()? AFAICT AllInstances() doesn't need > this part of the dance anyway, and instances() will be a bit easier to > understand. Okay, I'm moved this into Instances(). > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go > File environs/azure/environ_test.go (right): > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:101: var propertiesS1 = gwacl.HostedService{ > > Why is this a global variable? AFAICT it should be local to > patchWithPropertiesResponse (more about that below). The name would also be > less mystifying if this weren't a package-global variable. Fixed (see below). > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:126: c.Check(len(instances), Equals, 2) > > Please don't do this! The 2 is entirely arbitrary here, and relates to a global > variable that this test doesn't even mention. That's just building a timebomb > into your test. Maybe it's not so bad if the global is a single, well-known > constant, but here the connections go through the innards of a compound global. > Code isn't just instructions for the machine, it's also an explanation for > humans. So say what you mean, not just what you want the computer to do: always > use the appropriate len() expression for a thing like this. In the Unix > tradition, reviewers would put it as: "you misspelled > len(propertiesS1.Deployments)." Fixed. > If you want to keep the test maintainable, why not make the list of deployment > names a parameter to patchWithPropertiesResponse(), and build the HostedService > object there? Implicitly sharing data between the test and the factory through > a global variable is really bad practice. > > https://codereview.appspot.com/10732045/diff/1/environs/azure/environ_test.go... > environs/azure/environ_test.go:133: instances, err := > env.Instances([]instance.Id{"deployment-1"}) > > Here, too, you're implicitly referring to a global variable elsewhere that isn't > mentioned. It may seem like a nitpick now, while the test is (relatively) small > and still fresh in your mind and nobody else has modified it. But think kindly > of the engineer who is under pressure to fix a bug and isn't very familiar with > the code, because at some point it might be you. Fixed. > https://codereview.appspot.com/10732045/diff/1/environs/azure/instance.go > File environs/azure/instance.go (right): > > https://codereview.appspot.com/10732045/diff/1/environs/azure/instance.go#new... > environs/azure/instance.go:30: // An Azure deployment gets his DNS name when > it's created. > > Not "his" but "its"! > > Don't get into the habit of confusing "its" and "it's" though, or I shall have > to unfriend you on Friendface. :-) I would not want that to happen! Fixed! > https://codereview.appspot.com/10732045/diff/1/environs/azure/instance_test.go > File environs/azure/instance_test.go (right): > > https://codereview.appspot.com/10732045/diff/1/environs/azure/instance_test.g... > environs/azure/instance_test.go:18: // Deployment's hostnames are randomly > generated by Azure. > > If you mean hostnames of deployments (plural), then the apostrophe comes after > the s: "Deployments' hostnames." Fixed. > Don't we get to select our own hostnames? I thought we did. Maybe I'm > confusing hostnames and deployment names... No we do not get to select our own hostnames. > https://codereview.appspot.com/10732045/diff/1/environs/azure/instance_test.g... > environs/azure/instance_test.go:28: c.Check(azInstance.Id(), Equals, > instance.Id(deploymentName)) > > As in the environ name: don't weave implicit data relationships between > factories and tests through globals! Good point, fixed.
Sign in to reply to this message.
On 2013/06/28 15:55:37, rvb wrote: > If gwacl provider a ListAllDeployments, this would probably be a bit clearer. It has ListAllDeployments now!
Sign in to reply to this message.
On 2013/06/28 18:22:40, jtv.canonical wrote: > On 2013/06/28 15:55:37, rvb wrote: > > > If gwacl provider a ListAllDeployments, this would probably be a bit clearer. > > It has ListAllDeployments now! I've updated the code to cope with that change. Now, instead of a one complicated method we have two of them.
Sign in to reply to this message.
And also, conflicts. :( I'm sorry, that too is my fault. The easy part is that I changed the stub for Instances() for my StateInfo() test. Just keep your version and ignore mine. The other part is... not easy. Having a working Instances() means that StateInfo() now needs an x509 response patched in. I wrote up a fix, but can't easily integrate it with your branch because I branched off trunk, which contains all sorts of other changes since the version you branched from, including ones from outside the squad that we're not going to be too familiar with. So here's my suggestion: just disable TestStateInfo() by changing its name or bunging an early return in right at the top. (You could also comment out the code, but that might make the conflicts situation worse.) I'll follow up right away with my change.
Sign in to reply to this message.
On 2013/06/29 10:54:04, jtv.canonical wrote: > And also, conflicts. :( I'm sorry, that too is my fault. The easy part is > that I changed the stub for Instances() for my StateInfo() test. Just keep your > version and ignore mine. > > The other part is... not easy. Having a working Instances() means that > StateInfo() now needs an x509 response patched in. I wrote up a fix, but can't > easily integrate it with your branch because I branched off trunk, which > contains all sorts of other changes since the version you branched from, > including ones from outside the squad that we're not going to be too familiar > with. > > So here's my suggestion: just disable TestStateInfo() by changing its name or > bunging an early return in right at the top. (You could also comment out the > code, but that might make the conflicts situation worse.) I'll follow up right > away with my change. All right, I've renamed (and thus disabled) the test… I see I could probably fix the test now that Instances() and all the DNS methods are implemented but if you have a fix ready, I'll let you deal with it :).
Sign in to reply to this message.
LGTM by the way! Thanks for making the changes. Instances() and AllInstances() are long, but to me, easier to work with because cyclomatic complexity is reduced, there are fewer special cases, and there's no surprise when the slice of IDs you request happens to be empty. On a sidenote, the interface does not actually specify a nil return if the slice of requested IDs is empty — not saying that you should, but if you wanted you could just remove the special case. I don't think it specified a nil return in the past either, but it's something that was done in practice. What the interface *does* now say is that that we should return an ErrNoInstances error if none of the requested IDs were found! Only noticed that because I got curious after reading your code comment... It's an incompatible API change but it doesn't affect compilation. Let's worry about that afterwards... My previous note will probably show up minimized when you next read this page, so: remember to merge trunk and resolve conflicts; TestStateInfo() will panic but you can just disable it. Explanations above.
Sign in to reply to this message.
On 2013/06/29 11:52:39, jtv.canonical wrote: > LGTM by the way! Thanks for making the changes. Instances() and AllInstances() > are long, but to me, easier to work with because cyclomatic complexity is > reduced, there are fewer special cases, and there's no surprise when the slice > of IDs you request happens to be empty. > > On a sidenote, the interface does not actually specify a nil return if the slice > of requested IDs is empty — not saying that you should, but if you wanted you > could just remove the special case. I don't think it specified a nil return in > the past either, but it's something that was done in practice. I'd rather leave the special case: a) all the other providers do that b) with the ErrNoInstances error stuff and if I remove the special case, if you pass an empty list of instance ids, you'll get an error. That clearly does not feel right to me, if you give it a list of empty instances ids, no error should happen IMHO… > What the interface *does* now say is that that we should return an > ErrNoInstances error if none of the requested IDs were found! Only noticed that > because I got curious after reading your code comment... It's an incompatible > API change but it doesn't affect compilation. Let's worry about that > afterwards... Well, spotted, I've added that case (and a test). > My previous note will probably show up minimized when you next read this page, > so: remember to merge trunk and resolve conflicts; TestStateInfo() will panic > but you can just disable it. Explanations above. As said above, I've disabled that test for now.
Sign in to reply to this message.
> I'd rather leave the special case: > a) all the other providers do that > b) with the ErrNoInstances error stuff and if I remove the special case, if you > pass an empty list of instance ids, you'll get an error. That clearly does not > feel right to me, if you give it a list of empty instances ids, no error should > happen IMHO… Absolutely fine with me. Just mentioning it as an illustration, not as something I feel needs changing! > Well, spotted, I've added that case (and a test). I did the same for the MAAS provider, which was already implemented by the time this interface change was made, but was never updated to match it. It's up for review now. By the way, as far as I can see, we're probably expected to return ErrNoInstances even when zero instances are requested. Otherwise, StateInfo() may keep looping until its timeout in cases where no instances have been created yet, thinking that it's going to get them later. And oh, stop working on your weekend! :-)
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
