Code review comment for lp://staging/~sergiusens/snappy/rollYourOwnCorePersonal

On Mon, Jul 13, 2015 at 07:04:25AM -0000, Michael Vogt wrote:
> Review: Needs Information
>
> Thanks, this looks good! I have some question inline, probably mostly my own ignorance.
>
> Diff comments:
>
> > === modified file 'cmd/snappy/cmd_info.go'
> > --- cmd/snappy/cmd_info.go 2015-07-01 14:56:49 +0000
> > +++ cmd/snappy/cmd_info.go 2015-07-10 11:47:46 +0000
> > @@ -90,22 +91,12 @@
> > return nil
> > }
> >
> > -func ubuntuCoreChannel() string {
> > - parts, err := snappy.ActiveSnapsByType(pkg.TypeCore)
> > - if len(parts) == 1 && err == nil {
> > - return parts[0].Channel()
> > - }
> > -
> > - return "unknown"
> > -}
> > -
> > func info() error {
> > - release := ubuntuCoreChannel()
> > frameworks, _ := snappy.ActiveSnapNamesByType(pkg.TypeFramework)
> > apps, _ := snappy.ActiveSnapNamesByType(pkg.TypeApp)
> >
> > // TRANSLATORS: the %s release string
> > - fmt.Printf(i18n.G("release: %s\n"), release)
> > + fmt.Printf(i18n.G("release: %s\n"), release.String())
>
> This is nice (i.e. that all the code above gets killed :)
>

\o/

> > // TRANSLATORS: the %s an architecture string
> > fmt.Printf(i18n.G("architecture: %s\n"), snappy.Architecture())
> > // TRANSLATORS: the %s is a comma separated list of framework names
> >
> > === modified file 'release/release.go'
> > --- release/release.go 2015-05-15 13:33:27 +0000
> > +++ release/release.go 2015-07-10 11:47:46 +0000
> > @@ -51,9 +51,18 @@
> > return rel.String()
> > }
> >
> > +// Flavor returns the flavor for the release.
> > +func Flavor() string {
> > + return rel.flavor
> > +}
> > +
> > // Override sets up the release using a Release
> > -func Override(r Release) {
> > - rel = r
> > +func Override(flavor, series, channel string) {
>
> What the purpose of this function? AFAICT its only used in systemimage_test.go. Or is it there to allow overrides in the future? If thats the case, why will we need "overrides"? It seems like the channel.ini gives us the "ubuntu-{core,personal}" information as part of its Setup (or am I missing something)?
>

You forget about u-d-f and obtaining the first oem snap that has all the
info we need ;-)

> > + rel = Release{
> > + flavor: flavor,
> > + series: series,
> > + channel: channel,
> > + }
> > }
> >
> > // Setup is used to initialiaze the release information for the system
> >
> > === modified file 'snappy/systemimage.go'
> > --- snappy/systemimage.go 2015-07-01 14:48:33 +0000
> > +++ snappy/systemimage.go 2015-07-10 11:47:46 +0000
> > @@ -97,7 +98,7 @@
> >
> > // Name returns the name
> > func (s *SystemImagePart) Name() string {
> > - return systemImagePartName
> > + return fmt.Sprintf("ubuntu-%s", release.Flavor())
>
> Yay for this! Makes me wonder if we should move the "ubuntu-" string into release as well so that systemimage.go is a bit more agonstic about (but really minor minor).
>

Maybe so, but we also need to be backwards compatible here; if we can,
I'm all for it!

> > }
> >
> > // Origin returns the origin ("ubuntu")
>
>
> --
> https://code.launchpad.net/~sergiusens/snappy/rollYourOwnCorePersonal/+merge/264405
> You are the owner of lp:~sergiusens/snappy/rollYourOwnCorePersonal.