Issue 5610050: environs/ec2: add code to create juju-specific cloudinit data.
|
Created:
12 years, 3 months ago by rog Modified:
12 years, 3 months ago Reviewers:
mp+91062 Visibility:
Public. |
DescriptionWhen we create another provider, this code will be factored out,
but for the time being we'll keep it local to environs/ec2.
Also add a function to get ssh authorized keys.
https://code.launchpad.net/~rogpeppe/juju/go-ec2-cloudinit/+merge/91062
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : environs/ec2: add code to create juju-specific cloudinit data. #
Total comments: 39
Patch Set 3 : environs/ec2: add code to create juju-specific cloudinit data. #Patch Set 4 : environs/ec2: add code to create juju-specific cloudinit data. #
Total comments: 2
Patch Set 5 : environs/ec2: add code to create juju-specific cloudinit data. #Patch Set 6 : environs/ec2: add code to create juju-specific cloudinit data. #Patch Set 7 : environs/ec2: add code to create juju-specific cloudinit data. #
Total comments: 44
Patch Set 8 : environs/ec2: add code to create juju-specific cloudinit data. #Patch Set 9 : environs/ec2: add code to create juju-specific cloudinit data. #Patch Set 10 : environs/ec2: add code to create juju-specific cloudinit data. #
MessagesTotal messages: 19
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Mostly looks sensible, but I'm not sure about the authorized_keys stuff in particular. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode14 environs/ec2/auth.go:14: // transforms it into an identity of the form principle_name:hash that can be s/principle/principal/ https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode22 environs/ec2/auth.go:22: func isNotFound(e error) bool { Not sure about this, but "isNotFound(err)" reads very strangely to me, where "isNotFoundError(err)" is... well, it's clearly less compact, and also redundant, but to my mind still, er, "better". Hmm, how about "signifiesNotFound", or something like that? https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode41 environs/ec2/auth.go:41: // authorizedKeys finds an authorized_keys file and returns its contents "finds the user's public keys and returns the contents of a corresponding authorized_keys file"? https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode47 environs/ec2/auth.go:47: func authorizedKeys(path string) (string, error) { I'm wondering whether, for consistency's sake, this should be `paths []string` -- given that we combine all the user's (obviously accessible) public keys in the default case, it seems a little off to force them to construct an authorized_keys file themselves if they want anything different. OTOH, we're kinda hamstrung by the format of environments.yaml here, so I guess there's nothing we can do about it for now. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode50 environs/ec2/auth.go:50: files = []string{"id_dsa.pub", "id_rsa.pub", "identity.pub", "authorized_keys"} Why do we care what keys are authorized to connect to this local machine? Picking up the user's public keys makes perfect sense, but I don't see why we care about this. Transitive trust? :p https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode54 environs/ec2/auth.go:54: var finalError error Technically, I feel "firstError" may be more correct. Also, I'm not 100% sure about the logic -- if we get errors (other than NotFound errors) while collecting the keys, I kinda feel we ought to blow up (rather than just returning the arbitrary subset of intended keys that happened not to fail). https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode63 environs/ec2/auth.go:63: if finalError == nil && !isNotFound(err) { Yeah, I think something more like "!signifiesNotFound", unwieldy though it may be, reads far better than "!isNotFound". https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go File environs/ec2/cloudinit.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:20: // InstanceIdAccessor holds bash code that evaluates to the current instance id. Capitalisation? https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:23: // AdminSecret holds a secret that will be used to authenticate to zookeeper. Ditto https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:26: // ProviderType identifies the provider type so the host Ditto https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:30: // ZookeeperHosts lists the names of hosts running zookeeper. Ditto https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:37: // by examining the local environment. "jujuOrigin states what version of juju should be run on the instance"? Also not sure about "local environment", I know what you mean, but that phrase is a bit overloaded. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:40: // MachineId identifies the new machine. It must be Capitalisation https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:44: // SSHKeys specifies the keys that are allowed to Ditto https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:149: " --pidfile=/var/run/juju/machine-agent.pid", I have a branch queued up behind fix-update-charm that changes how this stuff works, and I'm not clear how we're going to keep these sorts of changes in sync. It's not a concern right now, but I am somewhat fretful about it. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:168: c.Assert(err, IsNil) Worth checking for a "#cloud-config" first line here. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:201: for _, test := range verifyTests { I do find this style of test a bit easier to read when the test data is next to the test method. Not idiomatic, I guess?
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
i've added a test for the policy parsing stuff. (it needs more, but i'm not sure of the various possibilities of the output). also changed jujuOrigin to be a value type, as it seemed more natural that way, https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode14 environs/ec2/auth.go:14: // transforms it into an identity of the form principle_name:hash that can be On 2012/02/01 17:17:34, fwereade wrote: > s/principle/principal/ Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode22 environs/ec2/auth.go:22: func isNotFound(e error) bool { On 2012/02/01 17:17:34, fwereade wrote: > Not sure about this, but "isNotFound(err)" reads very strangely to me, where > "isNotFoundError(err)" is... well, it's clearly less compact, and also > redundant, but to my mind still, er, "better". Hmm, how about > "signifiesNotFound", or something like that? i'm not keen on "signifies". i think i'll go with isNotFoundError. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode41 environs/ec2/auth.go:41: // authorizedKeys finds an authorized_keys file and returns its contents On 2012/02/01 17:17:34, fwereade wrote: > "finds the user's public keys and returns the contents of a corresponding > authorized_keys file"? not sure about this. corresponding to what? if path is specified, it does't find the user's public keys, it just opens that file. how about "authorizedKeys opens and returns the contents of a file containing ssh public keys (see sshd(8) for a description of the authorized_keys format)." https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode47 environs/ec2/auth.go:47: func authorizedKeys(path string) (string, error) { On 2012/02/01 17:17:34, fwereade wrote: > I'm wondering whether, for consistency's sake, this should be `paths []string` > -- given that we combine all the user's (obviously accessible) public keys in > the default case, it seems a little off to force them to construct an > authorized_keys file themselves if they want anything different. > > OTOH, we're kinda hamstrung by the format of environments.yaml here, so I guess > there's nothing we can do about it for now. yeah, i discussed this quite a bit previously with gustavo, and this was the result. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode50 environs/ec2/auth.go:50: files = []string{"id_dsa.pub", "id_rsa.pub", "identity.pub", "authorized_keys"} On 2012/02/01 17:17:34, fwereade wrote: > Why do we care what keys are authorized to connect to this local machine? > Picking up the user's public keys makes perfect sense, but I don't see why we > care about this. Transitive trust? :p good question. i copied the logic verbatim from providers/common/utils.py. arguments one way or the other are appreciated. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode54 environs/ec2/auth.go:54: var finalError error On 2012/02/01 17:17:34, fwereade wrote: > Technically, I feel "firstError" may be more correct. yes, i'll go with that. > Also, I'm not 100% sure about the logic -- if we get errors (other than NotFound > errors) while collecting the keys, I kinda feel we ought to blow up (rather than > just returning the arbitrary subset of intended keys that happened not to fail). depends whether any users make a habit of doing "chmod 000 $HOME/.ssh/someKeyFile" to turn off that key. i can see arguments both ways. if we don't care that it doesn't exist, should we care if there's a permission denied error, or whatever? we could return the error *and* the keys and let the caller decide. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go File environs/ec2/cloudinit.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:20: // InstanceIdAccessor holds bash code that evaluates to the current instance id. On 2012/02/01 17:17:34, fwereade wrote: > Capitalisation? Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:23: // AdminSecret holds a secret that will be used to authenticate to zookeeper. On 2012/02/01 17:17:34, fwereade wrote: > Ditto Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:26: // ProviderType identifies the provider type so the host On 2012/02/01 17:17:34, fwereade wrote: > Ditto Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:30: // ZookeeperHosts lists the names of hosts running zookeeper. On 2012/02/01 17:17:34, fwereade wrote: > Ditto Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:37: // by examining the local environment. On 2012/02/01 17:17:34, fwereade wrote: > "jujuOrigin states what version of juju should be run on the instance"? ok. > Also not sure about "local environment", I know what you mean, but that phrase > is a bit overloaded. agreed. got a better suggestion? https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:40: // MachineId identifies the new machine. It must be On 2012/02/01 17:17:34, fwereade wrote: > Capitalisation Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:44: // SSHKeys specifies the keys that are allowed to On 2012/02/01 17:17:34, fwereade wrote: > Ditto Done. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:168: c.Assert(err, IsNil) On 2012/02/01 17:17:34, fwereade wrote: > Worth checking for a "#cloud-config" first line here. that's already checked (many times!) in the cloudinit package. no point in testing it again IMO. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:201: for _, test := range verifyTests { On 2012/02/01 17:17:34, fwereade wrote: > I do find this style of test a bit easier to read when the test data is next to > the test method. Not idiomatic, I guess? no, that's reasonable. done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode41 environs/ec2/auth.go:41: // authorizedKeys finds an authorized_keys file and returns its contents On 2012/02/01 18:30:33, rog wrote: > On 2012/02/01 17:17:34, fwereade wrote: > > "finds the user's public keys and returns the contents of a corresponding > > authorized_keys file"? > > not sure about this. corresponding to what? if path is specified, it does't find > the user's public keys, it just opens that file. > > how about > "authorizedKeys opens and returns the contents of a file containing ssh public > keys (see sshd(8) for a description of the authorized_keys format)." Hmm, tbh, it feels like this function is somehow wrong (as is the python version). There are 3 places we could get an authorized_keys file for the instance: 1) verbatim copy from authorized-keys 2) verbatim read from authorized-keys-path (if relative, relative to ~/.ssh) 3) standard public keys in ~/.ssh The more I look at this, the more it seems that handling both authorized-keys-path and normal public keys in the same function (or, at least, code block) just adds complexity to no benefit; and so it seems to me that what we actually want is something like: // I can't think of a provider config that won't have to implement this interface... type KeysConfig interface { AuthorizedKeys() string AuthorizedKeysPath() string } func readKeys(path string) (string, error) { if !filepath.isabs(path) { path = filepath.Join(expandFileName(filepath.Join("~", ".ssh")), path) } data, err := ioutil.ReadFile(path) if err != nil { return nil, err } return string(data), nil } func authorizedKeys(config KeysConfig) (string, error) { if config.AuthorizedKeys() != "" { return config.AuthorizedKeys(), nil } else if config.AuthorizedKeysPath() != "" { // Hmm, can we do this? return readKeys(config.AuthorizedKeysPath()) } files = []string{"id_dsa.pub", "id_rsa.pub", "identity.pub"} for _, name := range(files) { // just readKeys(name) and collect or quick-return, whatever } // combine or error, whatever } The shape of the logic there feels much clearer than the python version ever did... and I think it'd be easier to document, too. Thoughts? https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode47 environs/ec2/auth.go:47: func authorizedKeys(path string) (string, error) { On 2012/02/01 18:30:33, rog wrote: > On 2012/02/01 17:17:34, fwereade wrote: > > I'm wondering whether, for consistency's sake, this should be `paths []string` > > -- given that we combine all the user's (obviously accessible) public keys in > > the default case, it seems a little off to force them to construct an > > authorized_keys file themselves if they want anything different. > > > > OTOH, we're kinda hamstrung by the format of environments.yaml here, so I > guess > > there's nothing we can do about it for now. > > yeah, i discussed this quite a bit previously with gustavo, and this was the > result. Fine by me :). https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode50 environs/ec2/auth.go:50: files = []string{"id_dsa.pub", "id_rsa.pub", "identity.pub", "authorized_keys"} On 2012/02/01 18:30:33, rog wrote: > On 2012/02/01 17:17:34, fwereade wrote: > > Why do we care what keys are authorized to connect to this local machine? > > Picking up the user's public keys makes perfect sense, but I don't see why we > > care about this. Transitive trust? :p > > good question. i copied the logic verbatim from providers/common/utils.py. > arguments one way or the other are appreciated. This isn't a copy of the logic in the python code (at least, not the code in trunk). 1) It checks authorized_keys, which the python version definitely doesn't (what it *does* do is check the config dict for a setting called "authorized-keys", and return that verbatim. However, that doesn't happen in this function at all; that's fine, because it seems that this function is designed to be called with authorized-keys*-path*, and that this would only happen if authorized-keys itself wasn't set.) 2) This version collects all the public keys in ~/.ssh, rather than just returning the first one found; that seems to me to be a reasonable thing to do, so no complaints, but it doesn't match the original ;). See earlier comment for further thoughts. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode54 environs/ec2/auth.go:54: var finalError error On 2012/02/01 18:30:33, rog wrote: > On 2012/02/01 17:17:34, fwereade wrote: > > Also, I'm not 100% sure about the logic -- if we get errors (other than > NotFound > > errors) while collecting the keys, I kinda feel we ought to blow up (rather > than > > just returning the arbitrary subset of intended keys that happened not to > fail). > > depends whether any users make a habit of doing "chmod 000 > $HOME/.ssh/someKeyFile" to turn off that key. > > i can see arguments both ways. if we don't care that it doesn't exist, should we > care if there's a permission denied error, or whatever? > > we could return the error *and* the keys and let the caller decide. Heh: if we want to do that, we should probably go the whole hog and do something like: authorizedKeys(path string) (string, []error) ...but that would be kinda hideous ;). On reflection, I'm happy with the existing approach. https://codereview.appspot.com/5610050/diff/4009/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/4009/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:227: // TODO add more tests with real output from apt_cache More would be good, indeed, but I like this.
Sign in to reply to this message.
https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode41 environs/ec2/auth.go:41: // authorizedKeys finds an authorized_keys file and returns its contents On 2012/02/01 21:39:32, fwereade wrote: > The shape of the logic there feels much clearer than the python version ever > did... and I think it'd be easier to document, too. Thoughts? Further to discussion on IRC, I can see valid arguments for your implementation, and I'm content either way ;).
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
PTAL gustavo? https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode41 environs/ec2/auth.go:41: // authorizedKeys finds an authorized_keys file and returns its contents On 2012/02/02 10:20:40, fwereade wrote: > On 2012/02/01 21:39:32, fwereade wrote: > > The shape of the logic there feels much clearer than the python version ever > > did... and I think it'd be easier to document, too. Thoughts? > > Further to discussion on IRC, I can see valid arguments for your implementation, > and I'm content either way ;). left as is. https://codereview.appspot.com/5610050/diff/3001/environs/ec2/auth.go#newcode50 environs/ec2/auth.go:50: files = []string{"id_dsa.pub", "id_rsa.pub", "identity.pub", "authorized_keys"} On 2012/02/01 21:39:32, fwereade wrote: > On 2012/02/01 18:30:33, rog wrote: > > On 2012/02/01 17:17:34, fwereade wrote: > > > Why do we care what keys are authorized to connect to this local machine? > > > Picking up the user's public keys makes perfect sense, but I don't see why > we > > > care about this. Transitive trust? :p > > > > good question. i copied the logic verbatim from providers/common/utils.py. > > arguments one way or the other are appreciated. > > This isn't a copy of the logic in the python code (at least, not the code in > trunk). > > 1) It checks authorized_keys, which the python version definitely doesn't (what > it *does* do is check the config dict for a setting called "authorized-keys", > and return that verbatim. However, that doesn't happen in this function at all; > that's fine, because it seems that this function is designed to be called with > authorized-keys*-path*, and that this would only happen if authorized-keys > itself wasn't set.) > > 2) This version collects all the public keys in ~/.ssh, rather than just > returning the first one found; that seems to me to be a reasonable thing to do, > so no complaints, but it doesn't match the original ;). > > See earlier comment for further thoughts. definitely an error that it included "authorized_keys". i've removed that. the collection of all keys was as decided in an earlier discussion with Gustavo on IRC. https://codereview.appspot.com/5610050/diff/4009/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/4009/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:227: // TODO add more tests with real output from apt_cache On 2012/02/01 21:39:32, fwereade wrote: > More would be good, indeed, but I like this. more done. stole 'em from the python version.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5610050/diff/8003/cloudinit/options.go File cloudinit/options.go (right): https://codereview.appspot.com/5610050/diff/8003/cloudinit/options.go#newcode6 cloudinit/options.go:6: // of the "launchpad.net/goyaml" Marshal function. // of the goyaml.Marshal function. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go#newcode21 environs/ec2/auth.go:21: func expandFileName(f string) string { s/expandFileName/expandTilde/ https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go#newcode48 environs/ec2/auth.go:48: f = filepath.Join(expandFileName(filepath.Join("~", ".ssh")), f) f = filepath.Join(os.Getenv("HOME"), ".ssh", f) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go#newcode52 environs/ec2/auth.go:52: if finalError == nil && !isNotFoundError(err) { "if finalError == nil" means finalError is actually the firstError. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go File environs/ec2/cloudinit.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:10: // cloudConfig represents initialization information for a new juju machine. This sounds like machineConfig rather than cloudConfig. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:69: return "cloud configuration requires " + string(e) I suggest something like this instead: "invalid machine configuration: missing " + string(e) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:189: // single-quote becomes single-quote, double-quote, single-quote, double-quote, single-quote Can we please get rid of that comment? You're transliterating the procedure below character by character. :-) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:219: // and the line itself, stripped of leading spaces. Please reindent. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:235: // returns the empty string and false. Please reindent. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:264: func policyToOrigin(policy string) jujuOrigin { Nice implementation. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:11: type cloudinitSuite struct{} This test suite is looking very good overall. I'm making some suggestions below mainly to reduce the verbosity. We should keep in mind some of these ideas for future tests. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:70: mdata, ok := mdata0.(map[interface{}]interface{}) I suggest we drop the Check/Assert in these cases. Just use: mdata := mdata0.(map[interface{}]interface{} Besides being cheap and clean, if that fails the panic will have very good information. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:78: func (t *cloudinitTest) checkOption(c *C, name string, value interface{}) { Using this doesn't look better than the straightforward: c.Check(t.x["apt_upgrade"], Equals, true) c.Check(t.x["apt_update"], Equals, true) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:86: if !c.Check(scripts0, NotNil, Bug("cloudinit has no entry for runcmd")) { We're not benefiting much from c.Check in those cases. This looks simpler/cleaner: if scripts0 == nil { c.Errorf("cloudinit has no entry for runcmd") return } https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:90: if !c.Check(ok, Equals, true, Bug("runcmd entry is wrong type; got %T want []interface{}", scripts0)) { scripts := scripts0.([]interface{}) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:96: s, ok := s0.(string) s := s0.(string) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:104: c.Check(found, Equals, true, Bug("script %q not found", pattern)) if !found { c.Errorf("script %q not found", pattern) } https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:110: if !c.Check(pkgs0, NotNil, Bug("cloudinit has no entry for packages")) { if pkgs == nil { c.Errorf(...) return } https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:114: pkgs, ok := pkgs0.([]interface{}) pkgs := pkgs0.([]interface{}) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:121: p, ok := p0.(string) p := p0.(string) https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:127: c.Check(found, Equals, true, Bug("%q not found in packages", pkg)) if !found { c.Errorf(...) } https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:141: // worrying about internal details of the cloudinit Sweet. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:290: }, {`N: VAT GEEV?`, This one is off in comparison to the rest.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/5610050/diff/8003/cloudinit/options.go File cloudinit/options.go (right): https://codereview.appspot.com/5610050/diff/8003/cloudinit/options.go#newcode6 cloudinit/options.go:6: // of the "launchpad.net/goyaml" Marshal function. On 2012/02/03 14:57:08, niemeyer wrote: > // of the goyaml.Marshal function. Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go File environs/ec2/auth.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go#newcode21 environs/ec2/auth.go:21: func expandFileName(f string) string { On 2012/02/03 14:57:08, niemeyer wrote: > s/expandFileName/expandTilde/ Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go#newcode48 environs/ec2/auth.go:48: f = filepath.Join(expandFileName(filepath.Join("~", ".ssh")), f) On 2012/02/03 14:57:08, niemeyer wrote: > f = filepath.Join(os.Getenv("HOME"), ".ssh", f) Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/auth.go#newcode52 environs/ec2/auth.go:52: if finalError == nil && !isNotFoundError(err) { On 2012/02/03 14:57:08, niemeyer wrote: > "if finalError == nil" means finalError is actually the firstError. yeah, i've changed it to "firstError" in a later branch (william's suggestion) but it hadn't made its way back here. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go File environs/ec2/cloudinit.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:10: // cloudConfig represents initialization information for a new juju machine. On 2012/02/03 14:57:08, niemeyer wrote: > This sounds like machineConfig rather than cloudConfig. Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:69: return "cloud configuration requires " + string(e) On 2012/02/03 14:57:08, niemeyer wrote: > I suggest something like this instead: > > "invalid machine configuration: missing " + string(e) Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:189: // single-quote becomes single-quote, double-quote, single-quote, double-quote, single-quote On 2012/02/03 14:57:08, niemeyer wrote: > Can we please get rid of that comment? You're transliterating the procedure > below character by character. :-) i realise that, but in some fonts (including my own) it's hard to distinguish "'" from '''''. (try pasting this paragraph into a google mail compose window for example). i'd be happy leaving it if that's ok. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:219: // and the line itself, stripped of leading spaces. On 2012/02/03 14:57:08, niemeyer wrote: > Please reindent. Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:235: // returns the empty string and false. On 2012/02/03 14:57:08, niemeyer wrote: > Please reindent. Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:70: mdata, ok := mdata0.(map[interface{}]interface{}) On 2012/02/03 14:57:08, niemeyer wrote: > I suggest we drop the Check/Assert in these cases. Just use: > > mdata := mdata0.(map[interface{}]interface{} > > Besides being cheap and clean, if that fails the panic will have very good > information. Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:78: func (t *cloudinitTest) checkOption(c *C, name string, value interface{}) { On 2012/02/03 14:57:08, niemeyer wrote: > Using this doesn't look better than the straightforward: > > c.Check(t.x["apt_upgrade"], Equals, true) > c.Check(t.x["apt_update"], Equals, true) Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:90: if !c.Check(ok, Equals, true, Bug("runcmd entry is wrong type; got %T want []interface{}", scripts0)) { On 2012/02/03 14:57:08, niemeyer wrote: > scripts := scripts0.([]interface{}) i have a slight objection to this in that it means that the given test is fatal, whereas before it would carry on running the rest of the tests. done anyway. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:96: s, ok := s0.(string) On 2012/02/03 14:57:08, niemeyer wrote: > s := s0.(string) Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:114: pkgs, ok := pkgs0.([]interface{}) On 2012/02/03 14:57:08, niemeyer wrote: > pkgs := pkgs0.([]interface{}) Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:121: p, ok := p0.(string) On 2012/02/03 14:57:08, niemeyer wrote: > p := p0.(string) Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:127: c.Check(found, Equals, true, Bug("%q not found in packages", pkg)) On 2012/02/03 14:57:08, niemeyer wrote: > if !found { > c.Errorf(...) > } Done. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:290: }, {`N: VAT GEEV?`, On 2012/02/03 14:57:08, niemeyer wrote: > This one is off in comparison to the rest. i just used the exact tests from the python code. deleted anyway.
Sign in to reply to this message.
LGTM with the test back. Thanks. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go File environs/ec2/cloudinit.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit.go#ne... environs/ec2/cloudinit.go:189: // single-quote becomes single-quote, double-quote, single-quote, double-quote, single-quote On 2012/02/03 15:31:26, rog wrote: > i'd be happy leaving it if that's ok. Sure. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:90: if !c.Check(ok, Equals, true, Bug("runcmd entry is wrong type; got %T want []interface{}", scripts0)) { On 2012/02/03 15:31:26, rog wrote: > i have a slight objection to this in that it means that the given test is fatal, > whereas before it would carry on running the rest of the tests. done anyway. I understand that, but it doesn't carry its weight. The resting state of the test is working, and when it does break we'll have very good debugging information to fix it, and may run tests again. Meanwhile, we may enjoy a shorter and cleaner test. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:290: }, {`N: VAT GEEV?`, On 2012/02/03 15:31:26, rog wrote: > On 2012/02/03 14:57:08, niemeyer wrote: > > This one is off in comparison to the rest. > > i just used the exact tests from the python code. > deleted anyway. Sorry for not being clear. I was just suggesting the indentation to be fixed so it looks like every other case. Please bring the test back.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
added test back. https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.go File environs/ec2/cloudinit_test.go (right): https://codereview.appspot.com/5610050/diff/8003/environs/ec2/cloudinit_test.... environs/ec2/cloudinit_test.go:290: }, {`N: VAT GEEV?`, On 2012/02/03 16:06:48, niemeyer wrote: > On 2012/02/03 15:31:26, rog wrote: > > On 2012/02/03 14:57:08, niemeyer wrote: > > > This one is off in comparison to the rest. > > > > i just used the exact tests from the python code. > > deleted anyway. > > Sorry for not being clear. I was just suggesting the indentation to be fixed so > it looks like every other case. Please bring the test back. Done.
Sign in to reply to this message.
*** Submitted: environs/ec2: add code to create juju-specific cloudinit data. When we create another provider, this code will be factored out, but for the time being we'll keep it local to environs/ec2. Also add a function to get ssh authorized keys. R=fwereade, niemeyer CC= https://codereview.appspot.com/5610050
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
