Add BundleSha256 to charm state

Add BundleSha256 to charm state



https://code.launchpad.net/~fwereade/juju/go-charm-sha256/+merge/105654

(do not edit description out of merge proposal)

[fwereade, 2012-05-14 12:58:44]

Please take a look.

[dave_cheney.net, 2012-05-14 21:03:05]

LGTM.

This is only mildly related to the change you have made, but what was the
rational for accessor methods for the private fields on a Bundle ?

[niemeyer, 2012-05-14 21:24:48]

https://codereview.appspot.com/6197085/diff/1/charm/bundle.go
File charm/bundle.go (right):

https://codereview.appspot.com/6197085/diff/1/charm/bundle.go#newcode151
charm/bundle.go:151: func (b *Bundle) Sha256() string {
I've purposefully left out this method when porting the code. We've seen the
assumption that the sha256 is a property of the charm in code and conversations
before, but it's really not. The same charm content, bundled twice, may very
easily produce different contents and thus different digests. For this reason,
I'd prefer to keep this logic outside of the charm package, and as a property of
the *file*. We may have real dir & bundle digests at some point, that will act
correctly in those situations.

In addition to that and as another useful point to keep in mind, note how the
change above is streaming the *whole* content for the charm, even if a single
file is read out of the bundle (e.g. metadata.yaml). The reason we use zip files
rather than tarballs is precisely so that we can make good use of the indexing
properties of zips.

As a last point of observation, the necessary logic is completely generic, and
deals with io.Reader equally well. The implementation of the store makes good
use of that fact by extracting the sha256 while the bundle is "passing by",
making it cheap.

[niemeyer, 2012-05-14 21:26:21]

On 2012/05/14 21:03:05, dfc wrote:
> LGTM.
> 
> This is only mildly related to the change you have made, but what was the
> rational for accessor methods for the private fields on a Bundle ?

There are two implementations of the same interface charm.Charm. Bundle is one
of them.

[fwereade, 2012-05-15 10:25:06]

On 2012/05/14 21:24:48, niemeyer wrote:
> https://codereview.appspot.com/6197085/diff/1/charm/bundle.go
> File charm/bundle.go (right):
> 
> https://codereview.appspot.com/6197085/diff/1/charm/bundle.go#newcode151
> charm/bundle.go:151: func (b *Bundle) Sha256() string {
> I've purposefully left out this method when porting the code. We've seen the
> assumption that the sha256 is a property of the charm in code and
conversations
> before, but it's really not. The same charm content, bundled twice, may very
> easily produce different contents and thus different digests. For this reason,
> I'd prefer to keep this logic outside of the charm package, and as a property
of
> the *file*. We may have real dir & bundle digests at some point, that will act
> correctly in those situations.
> 
> In addition to that and as another useful point to keep in mind, note how the
> change above is streaming the *whole* content for the charm, even if a single
> file is read out of the bundle (e.g. metadata.yaml). The reason we use zip
files
> rather than tarballs is precisely so that we can make good use of the indexing
> properties of zips.
> 
> As a last point of observation, the necessary logic is completely generic, and
> deals with io.Reader equally well. The implementation of the store makes good
> use of that fact by extracting the sha256 while the bundle is "passing by",
> making it cheap.

Fair enough; my thinking was that a digest was tied to a *bundle* rather than to
a charm, but that sounds like a sensible strategy. I'll rework this as seems
necessary.

[niemeyer, 2012-05-15 20:20:00]

Do you want to reuse this branch/CL to implement the logic in another way, or is
there another branch coming?

For now, marking as WIP. If you want to re-push, please mark it as Rejected in
the Merge Proposal in Launchpad.

[fwereade, 2012-05-16 07:32:27]

Please take a look.

[niemeyer, 2012-05-17 19:02:22]

LGTM, just trivials:

https://codereview.appspot.com/6197085/diff/2002/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/6197085/diff/2002/state/charm.go#newcode77
state/charm.go:77: func (c *Charm) Sha256() string {
Can we please name this as BundleSha256? We already have the same method in the
store.

https://codereview.appspot.com/6197085/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6197085/diff/2002/state/state.go#newcode108
state/state.go:108: Sha256:    bundleSHA256,
Let's pick one side of it: s/SHA/Sha/

[fwereade, 2012-05-18 10:06:46]

*** Submitted:

Add BundleSha256 to charm state

R=dfc, niemeyer
CC=
https://codereview.appspot.com/6197085

https://codereview.appspot.com/6197085/diff/2002/state/charm.go
File state/charm.go (right):

https://codereview.appspot.com/6197085/diff/2002/state/charm.go#newcode77
state/charm.go:77: func (c *Charm) Sha256() string {
On 2012/05/17 19:02:23, niemeyer wrote:
> Can we please name this as BundleSha256? We already have the same method in
the
> store.

Done.

https://codereview.appspot.com/6197085/diff/2002/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6197085/diff/2002/state/state.go#newcode108
state/state.go:108: Sha256:    bundleSHA256,
On 2012/05/17 19:02:23, niemeyer wrote:
> Let's pick one side of it: s/SHA/Sha/

Done.