Code review comment for lp://staging/~wallyworld/juju-core/oxygen-data-parse

Thanks for the review. I've implemented the suggestions but have a
question about the NewImageConstraint() method remark. See my inline
comment.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go
File environs/imagemetadata/simplestreams.go (right):

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode36
environs/imagemetadata/simplestreams.go:36: func
NewImageConstraint(region, endpoint, release, arch, stream string)
ImageConstraint {
On 2013/05/15 17:06:26, rog wrote:
> i'm not entirely convinced of the worth of this function - if i saw a
call to
> it, i'd have to look up which argument corresponded to which attribute
- a
> struct literal would be clearer.

The reason for the function is because the returned ImageConstraint
struct contains an embedded CloudSpec struct and I didn't want to expose
this implementation detail. If I removed this function, then the user
will need to type:

ImageConstraint{
     CloudSpec: CloudSpec{
         Endpoint: endpoint,
         Region: region,
     },
     Release: release,
     Arch: arch,
     Stream: stream,
}

The above seems kinda verbose and as I said, exposes the internals of
ImageConstraint. Are you ok with those issues if I remove the function?

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode74
environs/imagemetadata/simplestreams.go:74: var releaseVersions =
map[string]string{
On 2013/05/15 17:06:26, rog wrote:
> since this is a mutable global, this should be guarded with a
sync.Mutex.

> How about, rather than caching the image in the image constraint, you
read the
> distro info once (I think it's not unreasonable to assume that
> /usr/share/distro-info doesn't change after the program has started)
and just
> look up the info via the releaseVersions map.

> var (
> releaseVersionsMutex sync.Mutex
> updatedReleaseVersions bool
> releaseVersions map[string]string
> )

> func (id *ImageConstraint) Id() (s string, err error) {
> vers, err := releaseVersion(ic.Release)
> etc
> }

> func releaseVersion(release string) (string, error)
> releaseVersionsMutex.Lock()
> defer releaseVersionsMutex.Unlock()
> if vers, ok := releaseVersions[release]; ok {
> return vers, nil
> }
> if updatedReleaseVersions {
> return "", errNotFound
> }
> err := updateDistroInfo()
> updatedReleaseVersions = true
> if err != nil {
> return "", err
> }
> if vers, ok := releaseVersions[release]; ok {
> return vers, nil
> }
> return "", errNotFound
> }

Done.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode102
environs/imagemetadata/simplestreams.go:102: // the numeric version may
contain a LTS moniker so strip that out.
On 2013/05/15 17:06:26, rog wrote:
> if len(parts) < 3 {
> return fmt.Errorf("syntax error in ...")
> }
> (or just ignore lines with fewer than the right number of parts).

> we don't want to panic because the file isn't correctly formatted.

Done.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode145
environs/imagemetadata/simplestreams.go:145: // ImageMetadata records
matching the supplied region, arch etc.
On 2013/05/15 17:06:26, rog wrote:
> this comment seems out of place.
> how about just:

> // ImageMetadata holds information about a particular cloud image.
> ?

Done.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode184
environs/imagemetadata/simplestreams.go:184: // The index file location
is as specified.
On 2013/05/15 17:06:26, rog wrote:
> s/specified./specified. The usual file location is DefaultIndexPath./

Done.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode193
environs/imagemetadata/simplestreams.go:193: // fetchData gets all the
data from the given path relative to the given base URL.
On 2013/05/15 17:06:26, rog wrote:
> // It returns the data found and the URL used.

Done.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode224
environs/imagemetadata/simplestreams.go:224: return nil,
fmt.Errorf("cannot unmarshal JSON index metadata at URL %s: %s", url,
err.Error())
On 2013/05/15 17:06:26, rog wrote:
> return nil, fmt.Errorf("cannot unmarshal JSON index metadata at URL
%q: %v",
> url, err)

> (no need to explicitly call Error, and the url should be quoted)

Done.

https://codereview.appspot.com/9138044/diff/18001/environs/imagemetadata/simplestreams.go#newcode227
environs/imagemetadata/simplestreams.go:227: return nil,
fmt.Errorf("unexpected index file format %q, expected %s at URL %s",
indices.Format, format, url)
On 2013/05/15 17:06:26, rog wrote:
> s/%s/%q/g

Done.

https://codereview.appspot.com/9138044/