875559 - publish bors status queue

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[[e-mail] (:bhearsum)]

Attachment: publish bors status

This patch installs the files required to show the Bors status queue on the Buildbot WebStatus, and sets up a cronjob to regularly copy it there. It also refactors the toplevel modules a bit to give the Servo buildbot master+bors installation a single toplevel node - I think I read somewhere or someone told me that this is preferred.

Haven't tested it yet because I can't get the test env working on servo-puppet1 =(.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 753534 [details] [diff] [review]
publish bors status

Review of attachment 753534 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good, with just a few minor changes, but r- because it's not ready to land as-is.

Globally -- and I've missed this on previous reviews -- all files need MPL copyright headers.  If the JS comes from somewhere else, then that should be annotated too.

::: modules/bors/files/bors-render.js
@@ +49,5 @@
> +										who + " " + when + " #" + e["num_comments"], 
> +										"commentheader"));
> +			c.appendChild(elt_txt_class("div", what, "commentbody"));
> +			td.appendChild(c);
> +							 

lots of trailing whitespace here..

::: modules/bors/manifests/cron.pp
@@ +1,1 @@
> +define bors::cron($basedir, $owner, $status_location) {

why does bors::cron need the status locatoin?

::: modules/bors/manifests/status.pp
@@ +3,5 @@
> +        "${status_location}":
> +            owner => $owner,
> +            group => $group,
> +            mode => 755,
> +            ensure => directory;

This should probably depend on the buildmaster being set up somehow.  Ideally, that would be requires => Class['buildmaster'] or something of that sort.  That may require using some Anchor resource to ensure it works.

@@ +19,5 @@
> +            owner => $owner,
> +            group => $group,
> +            mode => 644,
> +            source => "puppet:///modules/bors/bors-render.js";
> +        "${status_location}/bors-render.js":

Duplicate resource

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Dustin J. Mitchell [:dustin] from comment #1)
> ::: modules/bors/manifests/cron.pp
> @@ +1,1 @@
> > +define bors::cron($basedir, $owner, $status_location) {
> 
> why does bors::cron need the status locatoin?

Because there's a cronjob that copies a file to $status_location. That's in bors-cron.erb, and I think I need the variable here in order for it to be accessible to that file...

> ::: modules/bors/manifests/status.pp
> @@ +3,5 @@
> > +        "${status_location}":
> > +            owner => $owner,
> > +            group => $group,
> > +            mode => 755,
> > +            ensure => directory;
> 
> This should probably depend on the buildmaster being set up somehow. 
> Ideally, that would be requires => Class['buildmaster'] or something of that
> sort.  That may require using some Anchor resource to ensure it works.

It seems wrong to for Bors module to assume that it will be interacting with Buildbot (because eg, it could be replaced with Jenkins or something else). Doesn't Bors::Instance["servo"] requiring Buildmaster::Buildbot_master::Servo["bms01-servo1"] cover this?

Comment 3

[[e-mail] (:bhearsum)]

Also, the bors js/css/html comes from a private github repo...how should I be annotating those files? Seems pointless to link to the repo.

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

A private repo, eh?  Yet, we're distributing it publicly?  What's the story there?  Is that private repo under the MPL?

Fair enough regarding the cronjob.  A comment might help someone else (or me, later) avoid confusion.

As for the dependency, it might be taken care of by the automatic dependencies of File resources on their parents.  I don't think the require will help - they're not as transitive as one hopes (so it will only make sure that the Bors::Instance resource is evaluated before the buildmaster, but not anything that the Bors::Instance resource requires -- that's what Anchors can fix).  If not, it just means that you run puppet twice, and add anchors later.

So those issues are fine.  The duplicate-resource and copyright-header issues stand.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Dustin J. Mitchell [:dustin] from comment #4)
> A private repo, eh?  Yet, we're distributing it publicly?  What's the story
> there?  Is that private repo under the MPL?

Heh, good point. It's not actually private code. Graydon originally put it in a private repo in hopes that it would reduce hoops he had to jump through when setting it up for Rust. There's no license attached to any of the upstream code.

> 
> As for the dependency, it might be taken care of by the automatic
> dependencies of File resources on their parents.  I don't think the require
> will help - they're not as transitive as one hopes (so it will only make
> sure that the Bors::Instance resource is evaluated before the buildmaster,
> but not anything that the Bors::Instance resource requires -- that's what
> Anchors can fix).  If not, it just means that you run puppet twice, and add
> anchors later.

Ah, OK.

> So those issues are fine.  The duplicate-resource and copyright-header
> issues stand.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: address comments

Added license headers to all of the puppet files, and fixed the duplicate resource (turns out that was copy/paste fail).

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 755906 [details] [diff] [review]
address comments

Looks good,  If you get info on the licensing of the web resources, please revisit :)

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 755906 [details] [diff] [review]
address comments

(In reply to Dustin J. Mitchell [:dustin] from comment #7)
> Comment on attachment 755906 [details] [diff] [review]
> address comments
> 
> Looks good,  If you get info on the licensing of the web resources, please
> revisit :)

Thanks, will do.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: fix bustage

Two things here:
1) stupid typo
2) publish bors status to a unique directory. I would've put public_html into dirs or something equivalent, but I don't think that's possible because it's created by a resource, not a class. I also had to use buildmaster::settings::master_root instead of master_basedir, because the latter only exists within the buildmaster module.

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 755926 [details] [diff] [review]
fix bustage

a=bustage anyway

Comment 11

[[e-mail] (:bhearsum)]

Comment on attachment 755926 [details] [diff] [review]
fix bustage

Landed to puppet320.

Comment 12

[[e-mail] (:bhearsum)]

Works fine now.