Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(13302)

Issue 12682045: Configurable simplestreams stream for Azure. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by jtv.canonical
Modified:
10 years, 8 months ago
Reviewers:
rvb, mp+179320, dimitern
Visibility:
Public.

Description

Configurable simplestreams stream for Azure. The design for this was worked out with Ian. We may want a configurable set of simplestreams base URLs at some point, but that is not a very user-friendly way to offer a configurable choice of streams. So instead, I kept the base URLs hard-coded (adding the "daily" base URL to the default "released" base URL) and added the option to select a stream. The imagemetadata package, when looking for an image, may look at all the URLs but only an image from the selected stream will match. In the process we also found that the documentation for imagemetadata.ImageConstaint.Stream was no longer accurate, so I updated it with Ian's consent. The new insights, and a change in the simplestreams format for Azure to make the product IDs consistent with those of other clouds, got this to the opint where it actually works with real simplestreams data. I updated the integration test we had for released images, to reflect the new format, and also added one for daily images. That may seem redundant, but some identifiers change not just their values but their formats depending on whether the stream is the released one or not. For the time being, the boilerplate config reflects the facts that: 1. We only have the "saucy" images working properly yet. 2. Since Saucy isn't released yet, we need the "daily" images. 3. The existing images seem to be tied to the West US location. I only just discovered that last fact, and it may get fixed soon. With this branch I can bootstrap an Azure environment, without specifying a hard-coded OS image. https://code.launchpad.net/~jtv/juju-core/az-daily-images/+merge/179320 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+143 lines, -61 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/azure/config.go View 5 chunks +19 lines, -1 line 2 comments Download
M environs/azure/environ.go View 2 chunks +5 lines, -7 lines 0 comments Download
M environs/azure/instancetype.go View 3 chunks +11 lines, -6 lines 2 comments Download
M environs/azure/instancetype_test.go View 7 chunks +102 lines, -46 lines 0 comments Download
M environs/imagemetadata/simplestreams.go View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 8
jtv.canonical
Please take a look.
10 years, 8 months ago (2013-08-09 04:35:01 UTC) #1
rvb
On 2013/08/09 04:35:01, jtv.canonical wrote: > Please take a look. LGTM. [0] 105 +//become more ...
10 years, 8 months ago (2013-08-09 09:25:16 UTC) #2
dimitern
LGTM with a few suggestions below https://codereview.appspot.com/12682045/diff/1/environs/azure/config.go File environs/azure/config.go (right): https://codereview.appspot.com/12682045/diff/1/environs/azure/config.go#newcode8 environs/azure/config.go:8: "io/ioutil" Blank line ...
10 years, 8 months ago (2013-08-09 09:52:35 UTC) #3
jtv.canonical
Thanks. All done. In some cases, in the other branches I had open. :)
10 years, 8 months ago (2013-08-09 10:06:43 UTC) #4
rvb
On 2013/08/09 10:06:43, jtv.canonical wrote: > Thanks. All done. In some cases, in the other ...
10 years, 8 months ago (2013-08-09 14:58:33 UTC) #5
jtv.canonical
On 2013/08/09 14:58:33, rvb wrote: > Quick note (not sure if this is the result ...
10 years, 8 months ago (2013-08-09 15:17:54 UTC) #6
rvb
> I'm not seeing those... how are you running the tests? cd environs/azure go test ...
10 years, 8 months ago (2013-08-09 15:33:16 UTC) #7
jtv.canonical
10 years, 8 months ago (2013-08-09 15:39:26 UTC) #8
Message was sent while issue was closed.
Nope, still not seeing them!  I am seeing a pre-existing log message (unknown
config field "unknown-future-setting") but not these new ones.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b