Support Archive, Builder, Build and PubHistory.

https://code.launchpad.net/~jani/lpad/lpad/+merge/88341

(do not edit description out of merge proposal)

[jani.monoses, 2012-01-12 10:57:02]

Please take a look.

[niemeyer, 2012-01-12 17:52:40]

This is really nice, thanks Jani.

Here are a few minor details for us to sort out before getting this in:

https://codereview.appspot.com/5535058/diff/1/archive.go
File archive.go (right):

https://codereview.appspot.com/5535058/diff/1/archive.go#newcode13
archive.go:13: // DisplayName returns an user friendly name of this archive
s/an/the/

... or ...

s/an/a/ and s/of/for/

https://codereview.appspot.com/5535058/diff/1/archive.go#newcode32
archive.go:32: // SelfLink returns the API URL of this Archive object
Please drop this method. This functionality is available through Value's
AbsLoc() method for all of the types. Just call a.AbsLoc() on your archive when
you need it.

https://codereview.appspot.com/5535058/diff/1/archive.go#newcode50
archive.go:50: "status":      "Published",
This interface means we'll need a separate method for each status. I suggest
this instead:

type PublishStatus string

const (
    PubPending    PublishStatus = "Pending"
    PubPublished  PublishStatus = "Published"
    PubSuperseded PublishStatus = "Superseded"
    PubDeleted    PublishStatus = "Deleted"
    PubObsolete   PublishStatus = "Obsolete"
}

func (a *Archive) PubHistory(source string, status PublishStatus)
(*PubHistoryList, error)

https://codereview.appspot.com/5535058/diff/1/archive_test.go
File archive_test.go (right):

https://codereview.appspot.com/5535058/diff/1/archive_test.go#newcode34
archive_test.go:34: func (s *ModelS) TestArchiveSources(c *C) {
s/TestArchiveSources/TestArchivePubHistory/

https://codereview.appspot.com/5535058/diff/1/build.go
File build.go (right):

https://codereview.appspot.com/5535058/diff/1/build.go#newcode21
build.go:21: type Pocket string
All of the documentation comments (for types, methods, etc) need proper
punctuation at the end.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode24
build.go:24: PocketAny       Pocket = ""
How's this being used?

https://codereview.appspot.com/5535058/diff/1/build.go#newcode49
build.go:49: // Build returns the package build identified by distro, package
name and id
// Build returns the identified package build.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode51
build.go:51: v, err := root.Location("/" + distro + "/+source/" + source + "/" +
strconv.Itoa(id)).Get(nil)
These values need to be escaped to avoid injection attacks:

distro = url.QueryEscape(distro)
source = url.QueryEscape(source)
path := fmt.Sprintf("/%s/+source/%s/%d", distro, source, id)

https://codereview.appspot.com/5535058/diff/1/build.go#newcode58
build.go:58: // Title is the title of this build
s/this build/build/ (referring to the receiver variable name in this case).

https://codereview.appspot.com/5535058/diff/1/build.go#newcode63
build.go:63: // ArchTag is the architecture of this build
If it's really the architecture, let's call this Arch() instead. What's the Tag
about?

https://codereview.appspot.com/5535058/diff/1/build.go#newcode68
build.go:68: // Retry gives back a failed build to the builder farm
// Retry sends a failed build back to the builder farm.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode74
build.go:74: // WebPage is the webpage of this build in Launchpad
s/this build/build/

https://codereview.appspot.com/5535058/diff/1/build.go#newcode79
build.go:79: // State returns the state of this build (successful, depwait,
failed, etc.)
s/this build/build/

https://codereview.appspot.com/5535058/diff/1/build.go#newcode80
build.go:80: func (build *Build) State() string {
Please drop the examples from the comment and use a properly typed result
variable:

func (build *Build) State() BuildState { ... }

https://codereview.appspot.com/5535058/diff/1/build.go#newcode89
build.go:89: // UploadLogURL is the URL of upload log if there was an upload
failure, None otherwise
s/of/of the/
s/, None otherwise//

https://codereview.appspot.com/5535058/diff/1/build.go#newcode94
build.go:94: // DateCreated is the date when this build was created
s/this build/build/

https://codereview.appspot.com/5535058/diff/1/build.go#newcode104
build.go:104: // SelfLink returns the API URL of this build entry
Drop it since we have AbsLoc.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode109
build.go:109: // The SPPH type holds the source package publication history
Let's please rename this type to PubHistory, and use "ph" for the receiver names
below rather than s.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode114
build.go:114: // PackageName gets the source package name of a SPPH
s/of a SPPH/of ph/

https://codereview.appspot.com/5535058/diff/1/build.go#newcode119
build.go:119: // PackageVersion gets the  package version of a SPPH
// PackageVersion returns the source package version of ph.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode124
build.go:124: // DistroSeries gets the distro series this is published in
// DistroSeries returns the distro series the packages are published into.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode133
build.go:133: // Archive gets the archive this is published in
// Archive returns the archive the packages are published into.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode142
build.go:142: // Component gets the component this package was published in
(i.e. main,universe)
// Component returns the component the packages are published into.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode147
build.go:147: // CurrentSourcePublicationLink gets the SPPH corresponding to a
Build
// PubHistory gets the source publication history corresponding to build.

Please move this method next to the other Build methods too.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode148
build.go:148: func (build *Build) CurrentSourcePublicationLink() (*SPPH, error)
{
s/CurrentSourcePublicationLink/PubHistory/g
s/*SPPH/*PubHistory/

https://codereview.appspot.com/5535058/diff/1/build.go#newcode156
build.go:156: // SPPHList is a list of SPPH objects used for iteration
// The PubHistoryList type represents a list of PubHistory objects.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode161
build.go:161: // For iterates over the list of SPPHs and calls f for each one.
s/SPPHs/*PubHistory values/

[jani.monoses, 2012-01-13 09:21:53]

Please take a look.

[jani.monoses, 2012-01-13 09:22:23]

https://codereview.appspot.com/5535058/diff/1/build.go
File build.go (right):

https://codereview.appspot.com/5535058/diff/1/build.go#newcode24
build.go:24: PocketAny       Pocket = ""
On 2012/01/12 17:52:40, niemeyer wrote:
> How's this being used?

Put here for future use.
PubHistory in archive.go hardcoded the common case of Release+Published but
pocket and status should be configurable. I just wanted to avoid passing too
many arguments, since the lack of keyword args makes it too verbose in the
common case, or so I thought. Status as made parametrizable now as per your
request in this review.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode63
build.go:63: // ArchTag is the architecture of this build
On 2012/01/12 17:52:40, niemeyer wrote:
> If it's really the architecture, let's call this Arch() instead. What's the
Tag
> about?

I tried keeping the LP API names whenever possible to make it easier reading for
those new to the code but familiar with the published API docs. I can change it,
no problem.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode89
build.go:89: // UploadLogURL is the URL of upload log if there was an upload
failure, None otherwise
On 2012/01/12 17:52:40, niemeyer wrote:
> s/of/of the/
> s/, None otherwise//

I put None in as that is the string LP returns on missing log, and those using
the function may want to check for this. It is not null or "" so it can
surprise. But I dropped the comment for now.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode156
build.go:156: // SPPHList is a list of SPPH objects used for iteration
On 2012/01/12 17:52:40, niemeyer wrote:
> // The PubHistoryList type represents a list of PubHistory objects.

I think I looked at the BugTaskList comment as an example.

[niemeyer, 2012-01-13 09:46:51]

Just one more small pass, and I think we can integrate.

https://codereview.appspot.com/5535058/diff/1/build.go
File build.go (right):

https://codereview.appspot.com/5535058/diff/1/build.go#newcode63
build.go:63: // ArchTag is the architecture of this build
On 2012/01/13 09:22:23, jani.monoses wrote:
> I tried keeping the LP API names whenever possible to make it easier reading
for
> those new to the code but familiar with the published API docs. I can change
it,
> no problem.

Keeping them on the same ball park is a good idea indeed, but in some cases the
names are awkward, possibly reflecting the organical growth of Launchpad. When
we can do clearly better, I'd rather do since most people will be looking at our
documentation first, rather than the Launchpad API's one.

That said, I'm not sure if this is the case here or not, to be honest. If
ArchTag is something like "386" or "amd64", then yes, I think Arch is the right
choice.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode89
build.go:89: // UploadLogURL is the URL of upload log if there was an upload
failure, None otherwise
On 2012/01/13 09:22:23, jani.monoses wrote:
> On 2012/01/12 17:52:40, niemeyer wrote:
> > s/of/of the/
> > s/, None otherwise//
> 
> I put None in as that is the string LP returns on missing log, and those using
> the function may want to check for this. It is not null or "" so it can
> surprise. But I dropped the comment for now.

You mean it returns the *string* "None"!? If so, this is a bug in Launchpad. If
you've tested and it's really the case, please check for that string within the
method, and transform it into "" before returning. This is what the developer
will expect.

https://codereview.appspot.com/5535058/diff/1/build.go#newcode156
build.go:156: // SPPHList is a list of SPPH objects used for iteration
On 2012/01/13 09:22:23, jani.monoses wrote:
> I think I looked at the BugTaskList comment as an example.

Indeed, it's possible there are similar comments elsewhere. I'll try to avoid
the explicit definition of "for iteration" from the docs of these types in the
future, since these details are visible from their interface.

The "is" vs. "represents" is a red herring, though. Both are fine.

https://codereview.appspot.com/5535058/diff/5001/build.go
File build.go (right):

https://codereview.appspot.com/5535058/diff/5001/build.go#newcode115
build.go:115: // PubHistory gets the the source publication history
corresponding to build.
s/gets the the/returns the/

https://codereview.appspot.com/5535058/diff/5001/build.go#newcode124
build.go:124: // PackageName gets the source package name of ph.
Sorry, i missed that one: s/gets/returns/

https://codereview.appspot.com/5535058/diff/5001/build.go#newcode129
build.go:129: // PackageVersion gets the  package version of ph.
This wasn't updated:

// PackageVersion returns the source package version of ph.

https://codereview.appspot.com/5535058/diff/5001/build.go#newcode143
build.go:143: // Archive returns the archive packages are published into.
Please use the suggested wording: s/packages/the packages/
Otherwise the sentence becomes ambiguous (returns the "archive packages"?).

[jani.monoses, 2012-01-13 10:45:07]

Please take a look.

[niemeyer, 2012-01-16 21:55:41]

LGTM, thank you!

[jani.monoses, 2012-01-16 22:42:42]

https://codereview.appspot.com/5535058/diff/1/build.go
File build.go (right):

https://codereview.appspot.com/5535058/diff/1/build.go#newcode89
build.go:89: // UploadLogURL is the URL of upload log if there was an upload
failure, None otherwise
On 2012/01/13 09:46:51, niemeyer wrote:
> On 2012/01/13 09:22:23, jani.monoses wrote:
> > On 2012/01/12 17:52:40, niemeyer wrote:
> > > s/of/of the/
> > > s/, None otherwise//
> > 
> > I put None in as that is the string LP returns on missing log, and those
using
> > the function may want to check for this. It is not null or "" so it can
> > surprise. But I dropped the comment for now.
> 
> You mean it returns the *string* "None"!? If so, this is a bug in Launchpad.
If
> you've tested and it's really the case, please check for that string within
the
> method, and transform it into "" before returning. This is what the developer
> will expect.

Seems OK, I may just have been confused by the docs initially. When I check now
it returns "" for most packages (as they do not have upload issues), so it is
ok.

While here I fixed the Build creation function, it expects a version number as
well. That is not an intuitive way of getting at build records, it is most often
by iterating the build records list of a certain package in an archive.

[niemeyer, 2012-01-17 00:34:26]

*** Submitted:

Support Archive, Builder, Build and PubHistory.

R=niemeyer
CC=
https://codereview.appspot.com/5535058