| wrtp | fwereade: review delivered. just a couple of documentation suggestions. | 11:39 |
|---|---|---|
| fwereade | wrtp, cool, tyvm | 11:40 |
| fwereade | wrtp, just one thing: It calls f.SetOutput(ioutil.Discard). | 11:42 |
| wrtp | fwereade: go on | 11:42 |
| fwereade | wrtp, would "It may call f.SetOutput(ioutil.Discard)." be acceptable? | 11:42 |
| fwereade | wrtp, it's the sense I was trying to convey in the original | 11:43 |
| fwereade | wrtp, or I could always make it call that and thereby get the simpler doc ;p | 11:43 |
| wrtp | fwereade: i'd maybe go for that | 11:43 |
| fwereade | wrtp, ...and more consistent behaviour, even if it doesn't matter much | 11:43 |
| wrtp | fwereade: exactly | 11:43 |
| fwereade | wrtp, sounds good, tyvm | 11:44 |
| wrtp | fwereade: cool | 11:44 |
| fwereade | wrtp, there's a trivial followup at https://codereview.appspot.com/6115048/ as well | 11:44 |
| * TheMue would like to review too but is currently involved in correcting the German translations of the 12.04 press releases. | 12:06 | |
| TheMue | wrtp, fwereade: moin btw | 12:06 |
| wrtp | TheMue: hiya | 12:06 |
| wrtp | fwereade: proposal for new method in the Environ interface: | 12:06 |
| wrtp | // UploadExecutables uploads the files in the | 12:06 |
| wrtp | // given directory to the environment. They will be | 12:06 |
| wrtp | // tagged with the given Juju version. | 12:06 |
| wrtp | UploadExecutables(dir string, version string) error | 12:06 |
| wrtp | fwereade: does that seem reasonable to you? | 12:07 |
| TheMue | So, back from translation office. *lol* | 12:50 |
| wrtp | fwereade: did you see my UploadExecutable proposal? | 13:05 |
| TheMue | wrtp: Btw, I wondered why my state test was failing. But then I get aware that I have to install sshd. Has been a surprise first. | 13:10 |
| wrtp | TheMue: slightly surprising it's not installed by default, yes. did it give a decent error message, BTW? | 13:11 |
| TheMue | wrtp: Yes, it told me that it could not start sshd. | 13:12 |
| wrtp | TheMue: that's good! | 13:12 |
| TheMue | wrtp: So fix just has been a sudo apt-get ... | 13:12 |
| TheMue | niemeyer: moin | 13:19 |
| niemeyer | Heya! | 13:21 |
| wrtp | niemeyer: yo! | 13:34 |
| wrtp | niemeyer: possible addition to the Environ interface: | 13:35 |
| wrtp | // UploadExecutables uploads the files in the | 13:35 |
| wrtp | // given directory to the environment. They will be | 13:35 |
| wrtp | // tagged with the given Juju version. | 13:35 |
| wrtp | UploadExecutables(dir string, version string) error | 13:35 |
| andrewsmedina | wrtp: hi | 13:37 |
| fwereade | wrtp, sorry, I missed that earlier | 13:39 |
| fwereade | wrtp, shouldn't dir be calculated from version? | 13:39 |
| wrtp | fwereade: no, because dir is just a temp directory into which we've installed the go executables | 13:39 |
| wrtp | niemeyer, fwereade: a possible alternative: | 13:40 |
| wrtp | // UploadExecutables uploads a gzipped tar archive | 13:40 |
| wrtp | // containing juju executables read from r. They will be | 13:40 |
| wrtp | // tagged with the given Juju version. | 13:40 |
| wrtp | UploadExecutables(r io.Reader, version string) error | 13:40 |
| wrtp | but i think i prefer the original. | 13:41 |
| wrtp | fwereade: perhaps i've misunderstood your comment though | 13:42 |
| niemeyer | wrtp: Hmm, sounds interesting, the latter feels better | 13:45 |
| niemeyer | wrtp: version shouldn't be a string, though | 13:45 |
| niemeyer | wrtp: UploadTools? | 13:45 |
| niemeyer | wrtp: as it goes onto tools/ | 13:45 |
| wrtp | niemeyer: the reason i prefer the former is that it gives the environ the choice to deploy in a different form if desired (e.g. local provider could just copy the dir) | 13:46 |
| wrtp | niemeyer: UploadTools sounds good. | 13:46 |
| wrtp | niemeyer: why shouldn't version be a string? | 13:46 |
| niemeyer | wrtp: I don't see how that's the case with the first and not the second | 13:46 |
| wrtp | niemeyer: the second one requires the executables to be tarred and gzipped already | 13:47 |
| wrtp | niemeyer: the first one just assumes that there's a directory containing them (as made by GOBIN=$dir go install launchpad.net/juju/go/cmd/...) | 13:47 |
| fwereade | wrtp, no, it was my misunderstanding | 13:47 |
| niemeyer | wrtp: Sounds sane.. I'd prefer to have that consistent | 13:47 |
| niemeyer | wrtp: There's no reason to have each environment with a different scheme | 13:48 |
| wrtp | niemeyer: ok. | 13:49 |
| niemeyer | wrtp: The alternative would be to reimplement the uploading logic everywhere, and then having to figure how each does what | 13:49 |
| niemeyer | wrtp: The consistency also makes it easier to have the other side consistent too (the consuming bit) | 13:50 |
| wrtp | niemeyer: i was thinking that there would be a function Archive(w io.Writer, dir string) which would do the tar & gzip | 13:50 |
| wrtp | niemeyer: then it's trivial for an environ to use it if it wants | 13:50 |
| niemeyer | wrtp: Until we figure why it would not want, this feels counterproductive | 13:51 |
| wrtp | niemeyer: one thing: you said that S3 doesn't count the file as uploaded if the network connection is interrupted. but what if the uploading client is interrupted (e.g. with ^C) ? | 13:52 |
| niemeyer | wrtp: That's what I did | 13:52 |
| wrtp | niemeyer: that's fine. | 13:52 |
| wrtp | niemeyer: hmm, so how does S3 know? | 13:53 |
| niemeyer | wrtp: http has a Content-Length header on the upload side too | 13:53 |
| wrtp | niemeyer: ah yes, i'd forgotten that. | 13:53 |
| wrtp | niemeyer: so, maybe the signature should be: UploadExecutables(r io.Reader, length int64, version string) error | 13:54 |
| niemeyer | wrtp: +1 | 13:54 |
| niemeyer | wrtp: Except for version.. it should be a Version IMO | 13:54 |
| wrtp | niemeyer: ok | 13:55 |
| wrtp | niemeyer: so... if we have a --force-upload flag, how can we tell the environ to bootstrap with the just-uploaded tools? | 13:56 |
| niemeyer | wrtp: We'll be sending the current version from the client.. we can use that to decide what to do | 13:58 |
| niemeyer | wrtp: We might also have a flag next to it.. something like "latest compatible" vs. "exact" | 13:58 |
| wrtp | niemeyer: but if the current version comes from a global variable, it won't be changing each time | 13:58 |
| wrtp | niemeyer: we could have an overriding environment variable | 13:58 |
| niemeyer | wrtp: Indeed.. what are you foreseeing there? | 13:58 |
| wrtp | niemeyer: i'm forseeing changing some code (but not the version) and uploading. i want that to work. | 13:59 |
| wrtp | niemeyer: rather, i don't want it to clash with other people using the same version. | 13:59 |
| niemeyer | wrtp: That sounds fine, that's what --force-upload would do, right? | 14:00 |
| wrtp | niemeyer: or are you envisaging everyone using a different bucket? | 14:00 |
| niemeyer | wrtp: Or --upload-tools, maybe | 14:00 |
| niemeyer | wrtp: Ah, yeah, definitely | 14:00 |
| niemeyer | wrtp: That was in the ML proposal | 14:00 |
| niemeyer | wrtp: There's a well known $PUBLIC, but in development mode it'd upload to your own bucket | 14:01 |
| wrtp | niemeyer: and that bucket comes from the environment.yaml, right? | 14:02 |
| niemeyer | wrtp: Right | 14:03 |
| niemeyer | wrtp: It's the same thing as the provider storage | 14:03 |
| wrtp | niemeyer: that should work. we can just use the existing control-bucket, i guess | 14:04 |
| niemeyer | wrtp: That's also the same thing as the provider storage :-) | 14:04 |
| wrtp | niemeyer: ok, just making sure i understand. | 14:04 |
| niemeyer | wrtp: Cool | 14:04 |
| wrtp | niemeyer: so --upload-tools triggers dev mode ? | 14:04 |
| niemeyer | wrtp: It's a bit unfortunate actually, that it's a common namespace | 14:04 |
| niemeyer | wrtp: We could have done better than that otherwise | 14:04 |
| niemeyer | wrtp: Hmm, good question.. what else would the dev mode affect? | 14:06 |
| wrtp | niemeyer: i can't think of anything else. | 14:06 |
| niemeyer | wrtp: Maybe we should handle that side of it as a feature on its own then | 14:06 |
| wrtp | niemeyer: i think --upload-tools *is* dev mode for all intents and purposes | 14:06 |
| niemeyer | wrtp: Just always check the provider storage before $PUBLIC on deployments | 14:06 |
| niemeyer | wrtp: and join the versions found for purposes of deployment.. then, pick the right version considering "exact" vs. "latest", and deploy the right one wherever it is | 14:07 |
| wrtp | niemeyer: i'm not sure i understand. if you've just uploaded the tools, why do you need to check anything other than provider storage for an exact match? | 14:09 |
| wrtp | niemeyer: or are you suggesting that normal user deplyments check their provider storage too? | 14:09 |
| niemeyer | wrtp: The fact the tools were uploaded sounds orthogonal to whether it is used, yeah.. When we don't upload the tools, what do we want to happen? | 14:10 |
| wrtp | niemeyer: that's a good question. | 14:10 |
| wrtp | niemeyer: i think that the tools should be chosen once, and then stored in the zk state | 14:10 |
| wrtp | niemeyer: (to be changed later if we want to upgrade) | 14:10 |
| niemeyer | wrtp: I'm not sure, only because the matrix is wider | 14:11 |
| niemeyer | wrtp: It's quite possible that different Ubuntu releases and architectures may be forced onto a specific version based on availability | 14:11 |
| wrtp | niemeyer: ah, but if we're uploading to the same URL each time, we have a problem, because we haven't got a good handle on the unchanging tools archive. | 14:11 |
| wrtp | niemeyer: hmm, that's true. but what should --upload-tools do in that case? | 14:12 |
| wrtp | niemeyer: if we can't use the version we're trying to upload, that is | 14:13 |
| niemeyer | wrtp: That's the thing, it feels like it's orthogonal.. it'd just cause the version in use to be sent to the environment storage | 14:13 |
| wrtp | s/we'retrying to upload/we've just uploaded/ | 14:13 |
| niemeyer | wrtp: Everything else works the same | 14:13 |
| niemeyer | wrtp: Then we have to define that everything else to be sensible, of course | 14:13 |
| wrtp | niemeyer: i'm not quite sure what you're suggesting here. that --dev-mode triggers exact version matching? | 14:15 |
| niemeyer | wrtp: No, I'm suggesting that this is orthogonal to the storage provider.. there are two different things: whether to upload the tools, and whether to enforce specific version matching or be lax | 14:17 |
| niemeyer | wrtp: So, just for the sake of understanding: bootstrap --upload-tools vs. bootstrap --exact-version vs. bootstrap --upload-tools --exact-version | 14:18 |
| wrtp | niemeyer: would there ever be a case where you'd want to do --upload-tools without --exact-version? | 14:19 |
| niemeyer | wrtp: Yes, in the cases where I'm happy for juju to pick a different version to enable me to work with other architectures, for example | 14:19 |
| wrtp | niemeyer: even though the version you've uploaded might well be ignored? | 14:20 |
| wrtp | only if there's a later uploaded version, i suppose | 14:20 |
| wrtp | i'm slightly nervous about having multiple deployed units all with different versions | 14:21 |
| wrtp | but i suppose it's an inevitable consequence of our versioning strategy | 14:21 |
| wrtp | *maybe* | 14:22 |
| niemeyer | wrtp: I understand and agree.. it's not even a consequence of our versioning strategy.. both of these aim at solving the problem which is supporting software for half a decade in multiple releases of the OS | 14:22 |
| niemeyer | wrtp: and in multiple architectures | 14:22 |
| niemeyer | wrtp: Practical example: | 14:22 |
| niemeyer | wrtp: 12.10 ships with juju 2.0.0.. then, we find an important bug, and release 2.0.1.. do we want the guy that is using the client of 2.0.0 because he happened to stumble upon the CD to deploy 2.0.0 or 2.0.1? I'd prefer if the servers were running 2.0.1, even if the version doesn't match | 14:24 |
| wrtp | niemeyer: yes, i agree. however, the scenario we were talking about was in development mode. i *think* i'd prefer --upload-tools to force my current version, even if it might not succeed on some architectures. | 14:26 |
| niemeyer | wrtp: Fair enough, I'm happy to start from there, with --upload-tools implying --exact-version | 14:27 |
| wrtp | niemeyer: so we give bootstrap a new exactVersion argument? | 14:29 |
| niemeyer | wrtp: +1 | 14:29 |
| wrtp | niemeyer: and Startinstance, presumably | 14:30 |
| niemeyer | wrtp: Actually, why don't we just start from there and ignore the other behavior for the moment? | 14:30 |
| wrtp | niemeyer: just use a given version; no compatibility comparisons? | 14:30 |
| TheMue | niemeyer: Beside an updated https://codereview.appspot.com/6120045/ I just proposed https://codereview.appspot.com/6111053/ containing only the NeedsUpgrade change (based on the trunk this time ;) ) | 14:30 |
| niemeyer | wrtp: Yeah, I *think* it'd be easy to get going with that, and then once the straightforward works, increment with a non-exact feature | 14:31 |
| niemeyer | wrtp: We can even switch the default without much pain, I suppose | 14:31 |
| wrtp | niemeyer: sounds good. | 14:31 |
| niemeyer | wrtp: Otherwise we'll end up stumbling upon several issues before we even have anything working | 14:32 |
| niemeyer | TheMue: Awesome, will try to review that now before lunch | 14:32 |
| wrtp | niemeyer: so, what should i do about the versions package? it pretty much implements semantic versions as specified on semver.org, so perhaps i should just take out the Compat func (which is juju-specific) and leave the rest, and use it just for the parsing and not the comparison. | 14:33 |
| TheMue | niemeyer: Great, thx | 14:33 |
| niemeyer | wrtp: IMO, it should be greatly simplified | 14:33 |
| wrtp | niemeyer: ok. you don't think it's useful having a standard semantic versions implementation? | 14:34 |
| niemeyer | wrtp: What we need is type Version stuct { Major, Minor, Patch int } | 14:34 |
| wrtp | niemeyer: ok, no pre-release, patch etc | 14:34 |
| niemeyer | wrtp: and a version.Parse that returns it | 14:35 |
| wrtp | i might just put the original onto rog-go.googlecode.org, as it's potentially useful for someone. | 14:35 |
| wrtp | niemeyer: sounds good. | 14:35 |
| niemeyer | wrtp: Agreed, not disagreeing with your generic design of semantic versioning itself | 14:35 |
| wrtp | niemeyer: it *is* pretty darn complex for what it gives you :-) | 14:36 |
| niemeyer | wrtp: Yeah | 14:36 |
| niemeyer | wrtp: func (v Version) IsDev() bool <= might be useful | 14:37 |
| wrtp | niemeyer: +1 | 14:37 |
| wrtp | niemeyer: (although i still think the odd-numbered versions will seem odd to people: "We've just released version 2.0.0 of juju" "But what happened to 1.0.0?!") | 14:38 |
| niemeyer | wrtp: odd/even is not so uncommon, and as long as we have the proper mechanisms in place in terms of code, no real damage can happen I suppose | 14:39 |
| niemeyer | wrtp: e.g. upgrade-juju shouldn't jump from stable to dev without a flag | 14:40 |
| wrtp | niemeyer: yeah, it's just a bit weird from a PR perspective. but if it's not uncommon, that's fine. | 14:40 |
| * TheMue loves LGTMs | 14:41 | |
| niemeyer | wrtp: http://en.wikipedia.org/wiki/Software_versioning#Odd-numbered_versions_for_development_releases | 14:41 |
| niemeyer | TheMue: ;) | 14:41 |
| wrtp | niemeyer: i can't see anything that does that on odd-numbered major releases. | 14:45 |
| wrtp | niemeyer: case in point: http://en.wikipedia.org/wiki/GNOME#Versions | 14:46 |
| niemeyer | wrtp: That doesn't bother me too much.. the main point of the odd major is being able to test the major upgrades before releasing them | 14:47 |
| wrtp | niemeyer: i'm sure it'll work out | 14:48 |
| TheMue | niemeyer: I would like to move all watchers into a new file "watchers.go" and rename "watch_test.go" to "watchers_test.go". OK for you? | 14:57 |
| wrtp | TheMue: one though: i think singular works just as well as plural, if not better. i'd suggest "watcher.go" and "watcher_test.go". | 14:59 |
| wrtp | s/though/thought/ | 14:59 |
| TheMue | wrtp: I think you have the better language feeling than me, so it would be ok for me to take "watcher", yes. | 15:00 |
| niemeyer | TheMue: Review delivered, cheers | 15:04 |
| niemeyer | TheMue: Yes, fine to move them elsewhere, as long as its in a branch by itself | 15:05 |
| niemeyer | TheMue: Hard to review moving code | 15:05 |
| * niemeyer => lunch, biab | 15:05 | |
| TheMue | niemeyer: Thx for the power reviewing. :D | 15:05 |
| TheMue | niemeyer: Yes, it will be an own branch. | 15:05 |
| TheMue | niemeyer: Enjoy | 15:06 |
| wrtp | niemeyer: i've updated the version CL taking into account our discussions above: https://codereview.appspot.com/6082044 | 15:57 |
| niemeyer | wrtp: Done | 16:14 |
| wrtp | niemeyer: submitted. thanks a lot. | 16:21 |
| niemeyer | wrtp: np! | 16:27 |
| wrtp | pwd | 16:31 |
| niemeyer | wrtp: I now realize that ClientVersion is probably not correct.. this should be version.Current I think | 16:37 |
| wrtp | niemeyer: +1 | 16:37 |
| wrtp | niemeyer: will do | 16:37 |
| niemeyer | wrtp: Thanks! | 16:37 |
| wrtp | fwereade: ping | 17:29 |
| fwereade | wrtp, pong | 21:29 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!