826753 - release automation should update ship it at certain points

Status: RESOLVED WONTFIX

(Release Engineering :: Release Automation: Other, defect, P3)

Description

[[e-mail] (:bhearsum)]

There's a few "major" events during release automation, and it'd be great to have the new kickoff system updated as we reach them. Specifically, I'm thinking of:
* Tagging completed
* All builds/repacks completed
* Updates on betatest
* Ready for releasetest
* Ready for release
* Postrelease

We may want to consider adding a new state to kickoff, too. Currently we have "ready" and "complete". It might be good to add "running" in the middle of that, and have releases marked as "running" when release runner starts them and have postrelease mark them as "complete".

Comment 1

[Rail Aliiev [:rail]]

Smells like TBPL for releases! :)

Comment 2

[[e-mail] (:bhearsum)]

Really want this, but it's P3.

Comment 3

[[e-mail] (:bhearsum)]

Alex suggested that it would be great to be able to see the timeline of a release on ship it (eg, tagging started at A, builds at B, updates at C) as well as how many failures there were. I think this ties in pretty closely to this.

Comment 5

[John Zeller [:zeller]]

Questions to gather clarification for this:
* /releases/<releaseName> gives a bunch of json info, including a variable called status, which includes "Pending". Is this the same as the state described in comment 0?
* Is the list in comment 0 sequentially accurate?
* Where are the scripts that normally generate the email updates for everything int he list from comment 0?
	* Do these scripts reference statusdb only?
* Should this timeline setup be a new endpoint like /status/<releaseName>?
	* JSON format option, but default to pretty graphical representation of timeline?
* Can I assume that the sequence of events in comment 0 should be organized in this status view as a timeline?

Comment 6

[Hal Wine [:hwine] use NI!]

(In reply to John Zeller [:zeller] from comment #5)
> Questions to gather clarification for this:
> * /releases/<releaseName> gives a bunch of json info, including a variable
> called status, which includes "Pending". Is this the same as the state
> described in comment 0?

No - everything in comment 0 is new to ship-it

> * Is the list in comment 0 sequentially accurate?

No - that's typical, but it could be out of order in real life

> * Where are the scripts that normally generate the email updates for
> everything int he list from comment 0?

All come from buildbot builder steps. They are events in the workflow.

> 	* Do these scripts reference statusdb only?
> * Should this timeline setup be a new endpoint like /status/<releaseName>?
> 	* JSON format option, but default to pretty graphical representation of
> timeline?

I'm not aware of a requirement to serve out status in phase 1, but check with others on that. Cool for phase 2. Primary goal of phase 1 is to collect & display status, aiui.

> * Can I assume that the sequence of events in comment 0 should be organized
> in this status view as a timeline?

Good question - currently there are only tabular views of releases. Proposals welcome.

Note that a common use case is for relman to kick off multiple releases at the same time, and want to monitor both. E.g. Firefox desktop & fennec.

Comment 7

[John Zeller [:zeller]]

(In reply to Hal Wine [:hwine] (use needinfo) from comment #6)
> (In reply to John Zeller [:zeller] from comment #5)
> > Questions to gather clarification for this:
> > * /releases/<releaseName> gives a bunch of json info, including a variable
> > called status, which includes "Pending". Is this the same as the state
> > described in comment 0?
> 
> No - everything in comment 0 is new to ship-it

Comment 0 mentions that, "Currently we have 'ready' and 'complete'." What is that referring to?

> > * Where are the scripts that normally generate the email updates for
> > everything int he list from comment 0?
> 
> All come from buildbot builder steps. They are events in the workflow.

So is a good course of action here to query statusdb to generate these updates? Or is there a service I can connect to?

> > * Should this timeline setup be a new endpoint like /status/<releaseName>?
> > 	* JSON format option, but default to pretty graphical representation of
> > timeline?
> 
> I'm not aware of a requirement to serve out status in phase 1, but check
> with others on that. Cool for phase 2. Primary goal of phase 1 is to collect
> & display status, aiui.

What's the difference between 'serve out status' and 'collect & display status'?

> > * Can I assume that the sequence of events in comment 0 should be organized
> > in this status view as a timeline?
> 
> Good question - currently there are only tabular views of releases.
> Proposals welcome.
> 
> Note that a common use case is for relman to kick off multiple releases at
> the same time, and want to monitor both. E.g. Firefox desktop & fennec.

I am trying to build a context here for what the deliverable is going to be. My first impression is a JSON or graphical endpoint that shows all releases, or a specific release.

Comment 8

[John Zeller [:zeller]]

Last Monday I scoped the steps for this bug, and spent the week prototyping everything to a functional state. I have now created a number of bugs to help with the steps needed to ship this functionality. Here are the steps as I have scoped them:

 1) Bug 1032975 - Add new table(s) to shipit database
 2) Bug 1032978 - Add a standalone process that listens to pulse for release related buildbot messages (shipit-agent)
 3) Bug 1032985 - Add REST API entry point to shipit that allows shipit-agent to enter release data into shipit database
 4) This bug - Add the following endpoints to the app
   - /status
     - Lists all available releases, that have status data, in JSON format and can be queried with parameters (ie /status?var1=1&var2=10). Analogous to /releases
   - /status.html
     - Pretty GUI view of the info shown in /status. Analogous to /releases.html
   - /status/<release-name>
     - GET: Lists release status info in JSON. Analogous to /releases/<release-name>
     - POST: covered in bug 1032985
   - /status/<release-name>.html
     - Pretty GUI view of the info shown in /status/<release-name> GET, possibly including a visual timeline of the 6 steps referred to in comment 3

More to come in the bugs from steps 1, 2, and 3

Comment 9

[John Zeller [:zeller]]

Attachment: bug826753.patch

This patch is *not* ready for feedback or review yet. Just a demonstration of where things are at currently.

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 8459678 [details] [diff] [review]
bug826753.patch

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

There's a lot below, but the tl;dr is "let's get the model sorted out, and the rest should flow from there". I think it would be good for you to revise the model (doesn't have to be implemented or tested - i'm just interested in what the tables and columns will look like) and then we should chat again to make sure we're both on the same page.

I do think this is coming along well, so don't be discouraged!

::: kickoff/model.py
@@ +189,5 @@
>      return releases
> +
> +
> +class ReleaseData(db.Model):
> +

Most of the data in this table is OK, but it needs a bit of a reorg. There's two main things here:
1) All of the release details things (product, version, build number, branch) should be removed, and name should be a reference to one of the releases tables. You won't be able to make this a true foreign key, because there's 3 different tables, but there's no reason to duplicate this information across here.
2) While all of the data currently comes in via Pulse, other sources may be useful in the future (eg, when we integrate post-release actions into Ship It, we'll probably want to update this table). So, I think this table should be generic "event" logging, not tied to Pulse's message model.

A few other comments on specific columns below:

@@ +193,5 @@
> +
> +    """A base class with all of the common columns for any pulse.m.o update message, 
> +    in this case from buildbot for releases."""
> +    __tablename__ = 'release_data'
> +    statusID = db.Column(db.Integer(), primary_key=True)

I don't see this ever getting used - it's probably not necessary. Builders are unique per release, so the PK here should probably be name+builderName (but see below re: builderName).

@@ +197,5 @@
> +    statusID = db.Column(db.Integer(), primary_key=True)
> +    name = db.Column(db.String(100))
> +    _submittedAt = db.Column(
> +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> +        default=datetime.utcnow)

This is the time the builder completed, and not a reflection of the submittedAt column in a Release table, right? If so, "eventTime" is probably a better name. The reason we use "submittedAt" for releases is because submission is the first step to starting a release. This table represents events as part of the release build process, so naming the column the same could be a source of confusion.

@@ +200,5 @@
> +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> +        default=datetime.utcnow)
> +    sent = db.Column(db.String(500), nullable=False)
> +    status_type = db.Column(db.String(500), nullable=False)
> +    routing_key = db.Column(db.String(500), nullable=False)

What are these three columns for? Are they used? Same for message_id and original_data. It seems like you're storing some things that Pulse gives you just because you have them. Depending what they are, that may or may not be useful. Basically, unless you have a need for the data, don't store it.

@@ +201,5 @@
> +        default=datetime.utcnow)
> +    sent = db.Column(db.String(500), nullable=False)
> +    status_type = db.Column(db.String(500), nullable=False)
> +    routing_key = db.Column(db.String(500), nullable=False)
> +    builderName = db.Column(db.String(500), nullable=False)

I think this would be better off as "event_name" or something similar. This way we can track buildbot builders, as well as non-buildbot actions.

@@ +202,5 @@
> +    sent = db.Column(db.String(500), nullable=False)
> +    status_type = db.Column(db.String(500), nullable=False)
> +    routing_key = db.Column(db.String(500), nullable=False)
> +    builderName = db.Column(db.String(500), nullable=False)
> +    platform = db.Column(db.String(500), nullable=True)

What's the purpose behind storing platform? This is already part of the builder names. If you're doing some stuff that requires you to ifdef or calculate things based on platform, it's probably okay to keep this.

@@ +204,5 @@
> +    routing_key = db.Column(db.String(500), nullable=False)
> +    builderName = db.Column(db.String(500), nullable=False)
> +    platform = db.Column(db.String(500), nullable=True)
> +    seqNum = db.Column(db.Integer(), nullable=True)
> +    seqTotal = db.Column(db.Integer(), nullable=True)

If you find yourself needing to do a lot of "if/else" around seqNum being null or not, you might consider defaulting both of these to "1" (and not allowing them to be nullable), and then even events that only have a single chunk will pass checks that make sure all of the chunks you need are finished.

Also, I'd recommend s/seq/chunk/g - that's the term we use everywhere else.

@@ +205,5 @@
> +    builderName = db.Column(db.String(500), nullable=False)
> +    platform = db.Column(db.String(500), nullable=True)
> +    seqNum = db.Column(db.Integer(), nullable=True)
> +    seqTotal = db.Column(db.Integer(), nullable=True)
> +    message_id = db.Column(db.Integer(), nullable=False)

Doesn't look like message_id is used, probably don't need to store it.

@@ +226,5 @@
> +
> +    def __init__(
> +        self, name, sent, status_type, routing_key, builderName, platform,
> +        message_id, product, version, build_number, branch, results=None,
> +        seqNum=0, seqTotal=0, original_data=None):

Please adjust indentation here - the fact that these args align with the body of the method makes it hard to separate the two.

@@ +264,5 @@
> +        return '<ReleaseData %r>' % self.name
> +
> +
> +class ReleaseStatus(db.Model):
> +

I don't think this table needs to exist at all. Almost everything in it can be calculated by looking at data in ReleaseData. Basically, I think you can move the logic from the renewStatus functions into helper methods on the ReleaseData table, or just helper functions in this file in general.

I think you probably will need to store enUSPlatforms, but that can be stored in the other table. See below for additional thoughts on how that gets submitted.

Databases are optimized to let you query them, don't worry about the perf :). We can cache results for periods of time if it becomes too expensive.

::: kickoff/templates/base.html
@@ +23,4 @@
>  <ul>
>  <li><a href="{{ url_for('submit_release') }}">Submit a new release</a></li>
>  <li><a href="{{ url_for('releases') }}">View releases</a></li>
> +<li><a href="{{ url_for('statuses') }}">View statuses</a></li>

I'm cool with this while you're still settling on the model and other things, but I think it should be integrated into releases.html before landing. Also note that a "completed" release is actually just a release whose automation is running - it doesn't mean that all of its builders have run. We should probably rename the current "completed" to "running" as part of this.

::: kickoff/views/status.py
@@ +1,1 @@
> +import logging

This will probably change a lot based on the comments about the model, as well as when it merges with the existing releases views. I'm not going to spend any time looking at it for now.

::: kickoff/views/submit.py
@@ +57,5 @@
> +        statusTable = getStatusTable()
> +        import pdb
> +        pdb.set_trace()
> +        status = statusTable.createNewStatus(
> +            release.name, json.dumps(getEnUsPlatforms(release.name, release.branch)))

I don't think this is a good way to get the enUSPlatforms. Instead, release runner should them along when it marks the release as "complete" (reminder: this means "automation is running" in current vernacular). It already has access to the entire release config, so you can get rid of all the helper methods for this.

Once your two new tables have been merged together and reference the existing releases tables, you shouldn't need to do anything here at all, in fact. The status page can know whether or not anything has happened by looking for release that exist in a releases table, and then seeing which (if any) rows in the status table exist.

::: shipit-agent.py
@@ +1,1 @@
> +#!/usr/bin/env python

We talked yesterday about whether or not using Pulse is the right way of doing this. I thought about this some more, and I think it's just as good if not better than any alternative I could come up with. This script will change a lot based on my comments about the model, so I'm going to hold off looking at it for now.

::: vendor/lib/python/mozilla/release/info.py
@@ +1,1 @@
>  import re

Per my comments in submit.py, I don't think you need any of this. If you do, these changes need be to made to build/tools/lib/python/release/info.py, and imported here. Nothing in vendor/ should be modified directly - this directory is for code imported from elsewhere.

Comment 11

[John Zeller [:zeller]]

> I do think this is coming along well, so don't be discouraged!

I shant! :)

> 
> ::: kickoff/model.py
> @@ +189,5 @@
> >      return releases
> > +
> > +
> > +class ReleaseData(db.Model):
> > +
> 
> Most of the data in this table is OK, but it needs a bit of a reorg. There's
> two main things here:
> 1) All of the release details things (product, version, build number,
> branch) should be removed, and name should be a reference to one of the
> releases tables. You won't be able to make this a true foreign key, because
> there's 3 different tables, but there's no reason to duplicate this
> information across here.

Looked at this possibility as well (foreign key). It made more sense to just store the name to associate with the *-release tables manually, rather than anything super fancy.

> 
> @@ +197,5 @@
> > +    statusID = db.Column(db.Integer(), primary_key=True)
> > +    name = db.Column(db.String(100))
> > +    _submittedAt = db.Column(
> > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > +        default=datetime.utcnow)
> 
> This is the time the builder completed, and not a reflection of the
> submittedAt column in a Release table, right? If so, "eventTime" is probably
> a better name. The reason we use "submittedAt" for releases is because
> submission is the first step to starting a release. This table represents
> events as part of the release build process, so naming the column the same
> could be a source of confusion.

submittedAt is set when the row is added to the table. sent is actually the time the pulse message was sent out. I was thinking only one is likely nexessary... probably sent

> 
> @@ +200,5 @@
> > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > +        default=datetime.utcnow)
> > +    sent = db.Column(db.String(500), nullable=False)
> > +    status_type = db.Column(db.String(500), nullable=False)
> > +    routing_key = db.Column(db.String(500), nullable=False)
> 
> What are these three columns for? Are they used? Same for message_id and
> original_data. It seems like you're storing some things that Pulse gives you
> just because you have them. Depending what they are, that may or may not be
> useful. Basically, unless you have a need for the data, don't store it.

status_type was used to make separating the 6 types that we are interested in easier. This made more sense when I was prepared for there to be more to it than simply checking the builderName, but it now seems like it might make sense to remove this. What do you think?

routing_key is mostly the same information as builderName, except that it includes a distinction between started and finished, which we may want for phase 2 finer grain detail. should we keep it or worry about adding it later?

> 
> @@ +202,5 @@
> > +    sent = db.Column(db.String(500), nullable=False)
> > +    status_type = db.Column(db.String(500), nullable=False)
> > +    routing_key = db.Column(db.String(500), nullable=False)
> > +    builderName = db.Column(db.String(500), nullable=False)
> > +    platform = db.Column(db.String(500), nullable=True)
> 
> What's the purpose behind storing platform? This is already part of the
> builder names. If you're doing some stuff that requires you to ifdef or
> calculate things based on platform, it's probably okay to keep this.

platform is parsed out of the list of lists in msg['payload']['build']['properties'] (where msg is the pulse message raw). platform is used to determine where we are in the steps of builds and updates (using enUSPlatforms to determine which of them is complete). This makes sense unless we want to store all the build properties as a dump, which I think is a poor use of the db.

> 
> @@ +204,5 @@
> > +    routing_key = db.Column(db.String(500), nullable=False)
> > +    builderName = db.Column(db.String(500), nullable=False)
> > +    platform = db.Column(db.String(500), nullable=True)
> > +    seqNum = db.Column(db.Integer(), nullable=True)
> > +    seqTotal = db.Column(db.Integer(), nullable=True)
> 
> If you find yourself needing to do a lot of "if/else" around seqNum being
> null or not, you might consider defaulting both of these to "1" (and not
> allowing them to be nullable), and then even events that only have a single
> chunk will pass checks that make sure all of the chunks you need are
> finished.
> 
> Also, I'd recommend s/seq/chunk/g - that's the term we use everywhere else.

I was actually considering defaulting to 0 here, but same difference I think.

I think seqNum and seqTotal could be squashed into a single Column called chunk, that looks like '5/6'. What do you think?

> 
> ::: kickoff/templates/base.html
> @@ +23,4 @@
> >  <ul>
> >  <li><a href="{{ url_for('submit_release') }}">Submit a new release</a></li>
> >  <li><a href="{{ url_for('releases') }}">View releases</a></li>
> > +<li><a href="{{ url_for('statuses') }}">View statuses</a></li>
> 
> I'm cool with this while you're still settling on the model and other
> things, but I think it should be integrated into releases.html before
> landing. Also note that a "completed" release is actually just a release
> whose automation is running - it doesn't mean that all of its builders have
> run. We should probably rename the current "completed" to "running" as part
> of this.

Okay, I was considering that as an alternative. That's fine. Once we settle the schema, I will have a few questions concerning how we'd like to show a table of running releases.

> 
> ::: kickoff/views/submit.py
> @@ +57,5 @@
> > +        statusTable = getStatusTable()
> > +        import pdb
> > +        pdb.set_trace()
> > +        status = statusTable.createNewStatus(
> > +            release.name, json.dumps(getEnUsPlatforms(release.name, release.branch)))
> 
> I don't think this is a good way to get the enUSPlatforms. Instead, release
> runner should them along when it marks the release as "complete" (reminder:
> this means "automation is running" in current vernacular). It already has
> access to the entire release config, so you can get rid of all the helper
> methods for this.
> 
> Once your two new tables have been merged together and reference the
> existing releases tables, you shouldn't need to do anything here at all, in
> fact. The status page can know whether or not anything has happened by
> looking for release that exist in a releases table, and then seeing which
> (if any) rows in the status table exist.

Ah okay, so release runner will enter the data when changing the status to "complete". Does this mean we should be storing enUSPlatforms as a new Column in the 3 *-release tables? I think that sounds like a smart move.


Another question about merging the 2 tables. Are you suggesting that all Columns currently in release_status be removed? If not, what do you think about adding the 6 status Columns to the 3 *-release tables? They'd start out as 0 for each one, when a new release is submitted. This would simplify our table view too in the releases.html tabs. Then the Column values would be updated each time a new release_data row shows up, keeping the overview intact.

Option 2 is of course just doing it all on the fly each time with a query and comparison across the up to 200+ rows in the release_data table.

Comment 12

[[e-mail] (:bhearsum)]

One thing that I forgot in my earlier comment: shipit agent should not live in the release-kickoff repository. I think the best place for it is in build/tools, right beside release-runner.py: https://github.com/mozilla/build-tools/tree/master/buildfarm/release -- that script might help you in learning how to use a config file instead of command line options, too.

(In reply to John Zeller [:zeller] from comment #11)
> > ::: kickoff/model.py
> > @@ +189,5 @@
> > >      return releases
> > > +
> > > +
> > > +class ReleaseData(db.Model):
> > > +
> > 
> > Most of the data in this table is OK, but it needs a bit of a reorg. There's
> > two main things here:
> > 1) All of the release details things (product, version, build number,
> > branch) should be removed, and name should be a reference to one of the
> > releases tables. You won't be able to make this a true foreign key, because
> > there's 3 different tables, but there's no reason to duplicate this
> > information across here.
> 
> Looked at this possibility as well (foreign key). It made more sense to just
> store the name to associate with the *-release tables manually, rather than
> anything super fancy.

Yeah. Because you'll be managing that relationship manually you'll need to be careful to check consistency at times. We don't generally DELETE rows though, so maybe it won't be a big deal.

> > 
> > @@ +197,5 @@
> > > +    statusID = db.Column(db.Integer(), primary_key=True)
> > > +    name = db.Column(db.String(100))
> > > +    _submittedAt = db.Column(
> > > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > > +        default=datetime.utcnow)
> > 
> > This is the time the builder completed, and not a reflection of the
> > submittedAt column in a Release table, right? If so, "eventTime" is probably
> > a better name. The reason we use "submittedAt" for releases is because
> > submission is the first step to starting a release. This table represents
> > events as part of the release build process, so naming the column the same
> > could be a source of confusion.
> 
> submittedAt is set when the row is added to the table. sent is actually the
> time the pulse message was sent out. I was thinking only one is likely
> nexessary... probably sent

Yeah, submittedAt won't make sense here with the new model.

> 
> > 
> > @@ +200,5 @@
> > > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > > +        default=datetime.utcnow)
> > > +    sent = db.Column(db.String(500), nullable=False)
> > > +    status_type = db.Column(db.String(500), nullable=False)
> > > +    routing_key = db.Column(db.String(500), nullable=False)
> > 
> > What are these three columns for? Are they used? Same for message_id and
> > original_data. It seems like you're storing some things that Pulse gives you
> > just because you have them. Depending what they are, that may or may not be
> > useful. Basically, unless you have a need for the data, don't store it.
> 
> status_type was used to make separating the 6 types that we are interested
> in easier. This made more sense when I was prepared for there to be more to
> it than simply checking the builderName, but it now seems like it might make
> sense to remove this. What do you think?

Ah, I see. So, I don't think status_type makes sense here for that purpose, primarily because many of the events aren't going to have a status_type. For example, we'll have an event when the source tarball builder finishes, but it doesn't to any of the status types. If you want to continue to have shortcuts for this, I think it belongs on the releases' tables. Right now we have "ready" and "complete" (which needs to be renamed, like I mentioned) columns on it, which are very similar to your status types -- they indicate a particular point a release is at. Your status types indicate points after "ready" and "complete.

How about: Add a "status" column to your model, which can be in any of the following states: "submitted", "ready", "running", "tag_complete", "builds_complete", "updates_complete", "ready_for_releasetest", "ready_for_release", "shipped". (These may need to be mapped to numbers.) We could get rid of the "ready" and "complete" columns in this case, since their information would be stored in the new "status" column.

Another alternative is to get rid of status_types altogether and calculate the current status at every page load (ie, call the renewStatus functions all the time). I'm fine with either approach.

> routing_key is mostly the same information as builderName, except that it
> includes a distinction between started and finished, which we may want for
> phase 2 finer grain detail. should we keep it or worry about adding it later?

I don't think we want to depend on Pulse for finer grained detail - not all status updates will necessarily come through it. I think eventName (nee builderName) is better for this.

> > @@ +202,5 @@
> > > +    sent = db.Column(db.String(500), nullable=False)
> > > +    status_type = db.Column(db.String(500), nullable=False)
> > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > +    builderName = db.Column(db.String(500), nullable=False)
> > > +    platform = db.Column(db.String(500), nullable=True)
> > 
> > What's the purpose behind storing platform? This is already part of the
> > builder names. If you're doing some stuff that requires you to ifdef or
> > calculate things based on platform, it's probably okay to keep this.
> 
> platform is parsed out of the list of lists in
> msg['payload']['build']['properties'] (where msg is the pulse message raw).
> platform is used to determine where we are in the steps of builds and
> updates (using enUSPlatforms to determine which of them is complete). This
> makes sense unless we want to store all the build properties as a dump,
> which I think is a poor use of the db.

OK, I see how it's used for calculating build progress. Are you sure it's used for calculating update progress, though? I know the "updates" builder sets a platform, but it's really a platform-independent step.

> > @@ +204,5 @@
> > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > +    builderName = db.Column(db.String(500), nullable=False)
> > > +    platform = db.Column(db.String(500), nullable=True)
> > > +    seqNum = db.Column(db.Integer(), nullable=True)
> > > +    seqTotal = db.Column(db.Integer(), nullable=True)
> > 
> > If you find yourself needing to do a lot of "if/else" around seqNum being
> > null or not, you might consider defaulting both of these to "1" (and not
> > allowing them to be nullable), and then even events that only have a single
> > chunk will pass checks that make sure all of the chunks you need are
> > finished.
> > 
> > Also, I'd recommend s/seq/chunk/g - that's the term we use everywhere else.
> 
> I was actually considering defaulting to 0 here, but same difference I think.
> 
> I think seqNum and seqTotal could be squashed into a single Column called
> chunk, that looks like '5/6'. What do you think?

Possibly, but it means you need to string parse it every time you want to use it.

> > ::: kickoff/templates/base.html
> > @@ +23,4 @@
> > >  <ul>
> > >  <li><a href="{{ url_for('submit_release') }}">Submit a new release</a></li>
> > >  <li><a href="{{ url_for('releases') }}">View releases</a></li>
> > > +<li><a href="{{ url_for('statuses') }}">View statuses</a></li>
> > 
> > I'm cool with this while you're still settling on the model and other
> > things, but I think it should be integrated into releases.html before
> > landing. Also note that a "completed" release is actually just a release
> > whose automation is running - it doesn't mean that all of its builders have
> > run. We should probably rename the current "completed" to "running" as part
> > of this.
> 
> Okay, I was considering that as an alternative. That's fine. Once we settle
> the schema, I will have a few questions concerning how we'd like to show a
> table of running releases.

Sounds fine. One thing that you could do to make things easier on you is to land this with statuses.html initially, and then you can merge them together in a follow-up patch. I'd be totally fine with that.

> > ::: kickoff/views/submit.py
> > @@ +57,5 @@
> > > +        statusTable = getStatusTable()
> > > +        import pdb
> > > +        pdb.set_trace()
> > > +        status = statusTable.createNewStatus(
> > > +            release.name, json.dumps(getEnUsPlatforms(release.name, release.branch)))
> > 
> > I don't think this is a good way to get the enUSPlatforms. Instead, release
> > runner should them along when it marks the release as "complete" (reminder:
> > this means "automation is running" in current vernacular). It already has
> > access to the entire release config, so you can get rid of all the helper
> > methods for this.
> > 
> > Once your two new tables have been merged together and reference the
> > existing releases tables, you shouldn't need to do anything here at all, in
> > fact. The status page can know whether or not anything has happened by
> > looking for release that exist in a releases table, and then seeing which
> > (if any) rows in the status table exist.
> 
> Ah okay, so release runner will enter the data when changing the status to
> "complete". Does this mean we should be storing enUSPlatforms as a new
> Column in the 3 *-release tables? I think that sounds like a smart move.

Yep, exactly. It's a property of a release, not a particular event - there's no reason to store it more than once per release.

> Another question about merging the 2 tables. Are you suggesting that all
> Columns currently in release_status be removed? If not, what do you think
> about adding the 6 status Columns to the 3 *-release tables? They'd start
> out as 0 for each one, when a new release is submitted. This would simplify
> our table view too in the releases.html tabs. Then the Column values would
> be updated each time a new release_data row shows up, keeping the overview
> intact.
>
> Option 2 is of course just doing it all on the fly each time with a query
> and comparison across the up to 200+ rows in the release_data table.

I think I answered this above when talking about status_type. Let me know if I didn't. In fact, it looks like we both came to similar conclusions here :).

Comment 13

[[e-mail] (:bhearsum)]

One more thing: ignore what I said about updates + platforms - I realize now that your "updates" is actually "update verify". However, we should probably also track the completion of the updates builder.

Comment 14

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #12)
> One thing that I forgot in my earlier comment: shipit agent should not live
> in the release-kickoff repository. I think the best place for it is in
> build/tools, right beside release-runner.py:
> https://github.com/mozilla/build-tools/tree/master/buildfarm/release -- that
> script might help you in learning how to use a config file instead of
> command line options, too.

Okay, I will add this to my todo

> 
> > 
> > > 
> > > @@ +200,5 @@
> > > > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > > > +        default=datetime.utcnow)
> > > > +    sent = db.Column(db.String(500), nullable=False)
> > > > +    status_type = db.Column(db.String(500), nullable=False)
> > > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > 
> > > What are these three columns for? Are they used? Same for message_id and
> > > original_data. It seems like you're storing some things that Pulse gives you
> > > just because you have them. Depending what they are, that may or may not be
> > > useful. Basically, unless you have a need for the data, don't store it.
> > 
> > status_type was used to make separating the 6 types that we are interested
> > in easier. This made more sense when I was prepared for there to be more to
> > it than simply checking the builderName, but it now seems like it might make
> > sense to remove this. What do you think?
> 
> Ah, I see. So, I don't think status_type makes sense here for that purpose,
> primarily because many of the events aren't going to have a status_type. For
> example, we'll have an event when the source tarball builder finishes, but
> it doesn't to any of the status types. If you want to continue to have
> shortcuts for this, I think it belongs on the releases' tables. Right now we
> have "ready" and "complete" (which needs to be renamed, like I mentioned)
> columns on it, which are very similar to your status types -- they indicate
> a particular point a release is at. Your status types indicate points after
> "ready" and "complete.
> 
> How about: Add a "status" column to your model, which can be in any of the
> following states: "submitted", "ready", "running", "tag_complete",
> "builds_complete", "updates_complete", "ready_for_releasetest",
> "ready_for_release", "shipped". (These may need to be mapped to numbers.) We
> could get rid of the "ready" and "complete" columns in this case, since
> their information would be stored in the new "status" column.
> 
> Another alternative is to get rid of status_types altogether and calculate
> the current status at every page load (ie, call the renewStatus functions
> all the time). I'm fine with either approach.

So if I understand correctly. You are suggesting having a single Column in each of the 3 *-release tables, which is a string holding the type of the *current* status?

If so, I have a few questions:
 - This approach iiuc assumes that a release is always in only 1 of these steps, is that the case? If it's not, what happens if builds are not done, and we've moved on to updates?
 - Would it be more effective to move all the rows from releaseStatus to the *-release tables?

> 
> > > @@ +202,5 @@
> > > > +    sent = db.Column(db.String(500), nullable=False)
> > > > +    status_type = db.Column(db.String(500), nullable=False)
> > > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > > +    builderName = db.Column(db.String(500), nullable=False)
> > > > +    platform = db.Column(db.String(500), nullable=True)
> > > 
> > > What's the purpose behind storing platform? This is already part of the
> > > builder names. If you're doing some stuff that requires you to ifdef or
> > > calculate things based on platform, it's probably okay to keep this.
> > 
> > platform is parsed out of the list of lists in
> > msg['payload']['build']['properties'] (where msg is the pulse message raw).
> > platform is used to determine where we are in the steps of builds and
> > updates (using enUSPlatforms to determine which of them is complete). This
> > makes sense unless we want to store all the build properties as a dump,
> > which I think is a poor use of the db.
> 
> OK, I see how it's used for calculating build progress. Are you sure it's
> used for calculating update progress, though? I know the "updates" builder
> sets a platform, but it's really a platform-independent step.

Via your next comment, yeah this is update_verify. How exactly do these events differ from updates? I agree we want to capture all those types of updates, but what do the "updates" you are thinking of look like? You say they are not platform dependent, so I am curious what they look like.

> 
> > Another question about merging the 2 tables. Are you suggesting that all
> > Columns currently in release_status be removed? If not, what do you think
> > about adding the 6 status Columns to the 3 *-release tables? They'd start
> > out as 0 for each one, when a new release is submitted. This would simplify
> > our table view too in the releases.html tabs. Then the Column values would
> > be updated each time a new release_data row shows up, keeping the overview
> > intact.
> >
> > Option 2 is of course just doing it all on the fly each time with a query
> > and comparison across the up to 200+ rows in the release_data table.
> 
> I think I answered this above when talking about status_type. Let me know if
> I didn't. In fact, it looks like we both came to similar conclusions here :).

My question regarding this is above. Additionally, I think we will probably be good just running helper functions to determine the statuses each time that status is requested.

Comment 15

[John Zeller [:zeller]]

Attachment: bug826753-model.patch

This includes most of your feedback. The other bit is dependent on the answers to the needinfo? above.

Comment 16

[[e-mail] (:bhearsum)]

I've got a couple of comments on the patch coming, but it sounds like we're pretty much on the same page now. I'd like to chat briefly today just to make sure we're on the same pgae re: the ReleaseStatus stuff.

(In reply to John Zeller [:zeller] from comment #14)
> (In reply to Ben Hearsum [:bhearsum] from comment #12)
> > > > @@ +200,5 @@
> > > > > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > > > > +        default=datetime.utcnow)
> > > > > +    sent = db.Column(db.String(500), nullable=False)
> > > > > +    status_type = db.Column(db.String(500), nullable=False)
> > > > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > > 
> > > > What are these three columns for? Are they used? Same for message_id and
> > > > original_data. It seems like you're storing some things that Pulse gives you
> > > > just because you have them. Depending what they are, that may or may not be
> > > > useful. Basically, unless you have a need for the data, don't store it.
> > > 
> > > status_type was used to make separating the 6 types that we are interested
> > > in easier. This made more sense when I was prepared for there to be more to
> > > it than simply checking the builderName, but it now seems like it might make
> > > sense to remove this. What do you think?
> > 
> > Ah, I see. So, I don't think status_type makes sense here for that purpose,
> > primarily because many of the events aren't going to have a status_type. For
> > example, we'll have an event when the source tarball builder finishes, but
> > it doesn't to any of the status types. If you want to continue to have
> > shortcuts for this, I think it belongs on the releases' tables. Right now we
> > have "ready" and "complete" (which needs to be renamed, like I mentioned)
> > columns on it, which are very similar to your status types -- they indicate
> > a particular point a release is at. Your status types indicate points after
> > "ready" and "complete.
> > 
> > How about: Add a "status" column to your model, which can be in any of the
> > following states: "submitted", "ready", "running", "tag_complete",
> > "builds_complete", "updates_complete", "ready_for_releasetest",
> > "ready_for_release", "shipped". (These may need to be mapped to numbers.) We
> > could get rid of the "ready" and "complete" columns in this case, since
> > their information would be stored in the new "status" column.
> > 
> > Another alternative is to get rid of status_types altogether and calculate
> > the current status at every page load (ie, call the renewStatus functions
> > all the time). I'm fine with either approach.
> 
> So if I understand correctly. You are suggesting having a single Column in
> each of the 3 *-release tables, which is a string holding the type of the
> *current* status?

Could be a string. Could also be a number that represents a state. I'm not too fussy about that part. I also noticed that we already have an existing "status" column on those tables, which is usually used to store some information about error states. That column is different than the one we're talking about here. Sorry for any confusion that caused - they're totally different things. We'll need a new name for this one.

> 
> If so, I have a few questions:
>  - This approach iiuc assumes that a release is always in only 1 of these
> steps, is that the case? If it's not, what happens if builds are not done,
> and we've moved on to updates?

Correct. These states are a sequence of events. A release starts at "submitted", and then sequentially goes through different states until reaching "shipped". Builds can't be generated until tagging is complete. Updates can't be generated until builds are complete, etc.

>  - Would it be more effective to move all the rows from releaseStatus to the
> *-release tables?

If you mean as an alternative to my "status" column idea above - yes, that's an option. I'm not as big of a fan of it, because it causes a bit of column explosion, but I wouldn't reject it outright.

> > > > @@ +202,5 @@
> > > > > +    sent = db.Column(db.String(500), nullable=False)
> > > > > +    status_type = db.Column(db.String(500), nullable=False)
> > > > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > > > +    builderName = db.Column(db.String(500), nullable=False)
> > > > > +    platform = db.Column(db.String(500), nullable=True)
> > > > 
> > > > What's the purpose behind storing platform? This is already part of the
> > > > builder names. If you're doing some stuff that requires you to ifdef or
> > > > calculate things based on platform, it's probably okay to keep this.
> > > 
> > > platform is parsed out of the list of lists in
> > > msg['payload']['build']['properties'] (where msg is the pulse message raw).
> > > platform is used to determine where we are in the steps of builds and
> > > updates (using enUSPlatforms to determine which of them is complete). This
> > > makes sense unless we want to store all the build properties as a dump,
> > > which I think is a poor use of the db.
> > 
> > OK, I see how it's used for calculating build progress. Are you sure it's
> > used for calculating update progress, though? I know the "updates" builder
> > sets a platform, but it's really a platform-independent step.
> 
> Via your next comment, yeah this is update_verify. How exactly do these
> events differ from updates? I agree we want to capture all those types of
> updates, but what do the "updates" you are thinking of look like? You say
> they are not platform dependent, so I am curious what they look like.

Updates is a single job in buildbot right now. After all the deliverables have been created (ie, all en-US and l10n builds done), we gather a bunch of metadata and publish it to the update server. When we say "updates are done/ready", what we mean is "updates are available to be tested on the update server".

Update verify is a set of jobs that run on each platform and do some stuff to verify the updates that were published in the above step.


> > > Another question about merging the 2 tables. Are you suggesting that all
> > > Columns currently in release_status be removed? If not, what do you think
> > > about adding the 6 status Columns to the 3 *-release tables? They'd start
> > > out as 0 for each one, when a new release is submitted. This would simplify
> > > our table view too in the releases.html tabs. Then the Column values would
> > > be updated each time a new release_data row shows up, keeping the overview
> > > intact.
> > >
> > > Option 2 is of course just doing it all on the fly each time with a query
> > > and comparison across the up to 200+ rows in the release_data table.
> > 
> > I think I answered this above when talking about status_type. Let me know if
> > I didn't. In fact, it looks like we both came to similar conclusions here :).
> 
> My question regarding this is above. Additionally, I think we will probably
> be good just running helper functions to determine the statuses each time
> that status is requested.

If we're going to calculate on the fly, we don't need the "status" column in the existing releases' tables.

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 8461239 [details] [diff] [review]
bug826753-model.patch

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

::: kickoff/model.py
@@ +41,4 @@
>  
>      def __init__(self, submitter, version, buildNumber, branch,
>                   mozillaRevision, l10nChangesets, dashboardCheck,
> +                 mozillaRelbranch, enUSPlatforms, submittedAt=None):

I don't think enUSPlatforms can be made mandatory here. This class needs to be init'ed at submission time (http://git.mozilla.org/?p=build/release-kickoff.git;a=blob;f=kickoff/views/submit.py;h=5c3197bc9af6f4c2bd88b05fe5a82af4e335db88;hb=HEAD#l50), and enUSPlatforms won't be known at that point.

@@ +190,5 @@
>                  releases.append(r)
>      return releases
> +
> +
> +class ReleaseData(db.Model):

What do you think about calling this ReleaseEvents instead?

@@ +196,5 @@
> +    """A base class with all of the common columns for any pulse.m.o update message, 
> +    in this case from buildbot for releases."""
> +    __tablename__ = 'release_data'
> +    unique_name = db.Column(db.String(500), nullable=False, primary_key=True)
> +    name = db.Column(db.String(100), nullable=False)

Why is unique_name necessary? Shouldn't name be unique already? (It is in the Releases' tables)

@@ +197,5 @@
> +    in this case from buildbot for releases."""
> +    __tablename__ = 'release_data'
> +    unique_name = db.Column(db.String(500), nullable=False, primary_key=True)
> +    name = db.Column(db.String(100), nullable=False)
> +    sent = db.Column(db.String(500), nullable=False)

Per earlier comments, this is the time the event was sent out. This should be a DateTime, like submittedAt is in the other tables. Make sure it always gets converted to UTC before storing :).

@@ +256,5 @@
> +    return statuses
> +
> +
> +def getStatusTable():
> +    return ReleaseStatus

I think getStatus and getStatusTable are both dead code now...that table is gone!

Comment 18

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #16)
> I've got a couple of comments on the patch coming, but it sounds like we're
> pretty much on the same page now. I'd like to chat briefly today just to
> make sure we're on the same pgae re: the ReleaseStatus stuff.

I will request a time.

> 
> (In reply to John Zeller [:zeller] from comment #14)
> > (In reply to Ben Hearsum [:bhearsum] from comment #12)
> > > > > @@ +200,5 @@
> > > > > > +        'submittedAt', db.DateTime(pytz.utc), nullable=False,
> > > > > > +        default=datetime.utcnow)
> > > > > > +    sent = db.Column(db.String(500), nullable=False)
> > > > > > +    status_type = db.Column(db.String(500), nullable=False)
> > > > > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > > > 
> > > > > What are these three columns for? Are they used? Same for message_id and
> > > > > original_data. It seems like you're storing some things that Pulse gives you
> > > > > just because you have them. Depending what they are, that may or may not be
> > > > > useful. Basically, unless you have a need for the data, don't store it.
> > > > 
> > > > status_type was used to make separating the 6 types that we are interested
> > > > in easier. This made more sense when I was prepared for there to be more to
> > > > it than simply checking the builderName, but it now seems like it might make
> > > > sense to remove this. What do you think?
> > > 
> > > Ah, I see. So, I don't think status_type makes sense here for that purpose,
> > > primarily because many of the events aren't going to have a status_type. For
> > > example, we'll have an event when the source tarball builder finishes, but
> > > it doesn't to any of the status types. If you want to continue to have
> > > shortcuts for this, I think it belongs on the releases' tables. Right now we
> > > have "ready" and "complete" (which needs to be renamed, like I mentioned)
> > > columns on it, which are very similar to your status types -- they indicate
> > > a particular point a release is at. Your status types indicate points after
> > > "ready" and "complete.
> > > 
> > > How about: Add a "status" column to your model, which can be in any of the
> > > following states: "submitted", "ready", "running", "tag_complete",
> > > "builds_complete", "updates_complete", "ready_for_releasetest",
> > > "ready_for_release", "shipped". (These may need to be mapped to numbers.) We
> > > could get rid of the "ready" and "complete" columns in this case, since
> > > their information would be stored in the new "status" column.
> > > 
> > > Another alternative is to get rid of status_types altogether and calculate
> > > the current status at every page load (ie, call the renewStatus functions
> > > all the time). I'm fine with either approach.
> > 
> > So if I understand correctly. You are suggesting having a single Column in
> > each of the 3 *-release tables, which is a string holding the type of the
> > *current* status?
> 
> Could be a string. Could also be a number that represents a state. I'm not
> too fussy about that part. I also noticed that we already have an existing
> "status" column on those tables, which is usually used to store some
> information about error states. That column is different than the one we're
> talking about here. Sorry for any confusion that caused - they're totally
> different things. We'll need a new name for this one.

A number works fine, and nah no confusion there. I actually hadn't noticed that yet, but thanks for heading me off at the pass before I was confused haha

> 
> > 
> > If so, I have a few questions:
> >  - This approach iiuc assumes that a release is always in only 1 of these
> > steps, is that the case? If it's not, what happens if builds are not done,
> > and we've moved on to updates?
> 
> Correct. These states are a sequence of events. A release starts at
> "submitted", and then sequentially goes through different states until
> reaching "shipped". Builds can't be generated until tagging is complete.
> Updates can't be generated until builds are complete, etc.
> 
> >  - Would it be more effective to move all the rows from releaseStatus to the
> > *-release tables?
> 
> If you mean as an alternative to my "status" column idea above - yes, that's
> an option. I'm not as big of a fan of it, because it causes a bit of column
> explosion, but I wouldn't reject it outright.

As long as these events are *always* in sequence, and *never* interleaf, I am totally fine with the 1. I was under the mistaken impression that I was seeking to explicitly allow for interleaving of these steps. If they are sequential, then definitely, let's just use the 1 column.

> 
> > > > > @@ +202,5 @@
> > > > > > +    sent = db.Column(db.String(500), nullable=False)
> > > > > > +    status_type = db.Column(db.String(500), nullable=False)
> > > > > > +    routing_key = db.Column(db.String(500), nullable=False)
> > > > > > +    builderName = db.Column(db.String(500), nullable=False)
> > > > > > +    platform = db.Column(db.String(500), nullable=True)
> > > > > 
> > > > > What's the purpose behind storing platform? This is already part of the
> > > > > builder names. If you're doing some stuff that requires you to ifdef or
> > > > > calculate things based on platform, it's probably okay to keep this.
> > > > 
> > > > platform is parsed out of the list of lists in
> > > > msg['payload']['build']['properties'] (where msg is the pulse message raw).
> > > > platform is used to determine where we are in the steps of builds and
> > > > updates (using enUSPlatforms to determine which of them is complete). This
> > > > makes sense unless we want to store all the build properties as a dump,
> > > > which I think is a poor use of the db.
> > > 
> > > OK, I see how it's used for calculating build progress. Are you sure it's
> > > used for calculating update progress, though? I know the "updates" builder
> > > sets a platform, but it's really a platform-independent step.
> > 
> > Via your next comment, yeah this is update_verify. How exactly do these
> > events differ from updates? I agree we want to capture all those types of
> > updates, but what do the "updates" you are thinking of look like? You say
> > they are not platform dependent, so I am curious what they look like.
> 
> Updates is a single job in buildbot right now. After all the deliverables
> have been created (ie, all en-US and l10n builds done), we gather a bunch of
> metadata and publish it to the update server. When we say "updates are
> done/ready", what we mean is "updates are available to be tested on the
> update server".
> 
> Update verify is a set of jobs that run on each platform and do some stuff
> to verify the updates that were published in the above step.

Oh okay excellent. So is it safe to say that I can expect to see only 1 'update' message, meaning 'updates are available to be tested on the update server'? And then after that, the verifies come... so then are you good with calling out 'Update' step the 1 update message, followed by all the update_verify before calling 'Update' done?

> 
> 
> > > > Another question about merging the 2 tables. Are you suggesting that all
> > > > Columns currently in release_status be removed? If not, what do you think
> > > > about adding the 6 status Columns to the 3 *-release tables? They'd start
> > > > out as 0 for each one, when a new release is submitted. This would simplify
> > > > our table view too in the releases.html tabs. Then the Column values would
> > > > be updated each time a new release_data row shows up, keeping the overview
> > > > intact.
> > > >
> > > > Option 2 is of course just doing it all on the fly each time with a query
> > > > and comparison across the up to 200+ rows in the release_data table.
> > > 
> > > I think I answered this above when talking about status_type. Let me know if
> > > I didn't. In fact, it looks like we both came to similar conclusions here :).
> > 
> > My question regarding this is above. Additionally, I think we will probably
> > be good just running helper functions to determine the statuses each time
> > that status is requested.
> 
> If we're going to calculate on the fly, we don't need the "status" column in
> the existing releases' tables.

In this case let's just do it all on the fly and leave out the "status" column, because I need to calculate the current details of a step anyway.

Comment 19

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #17)
> Comment on attachment 8461239 [details] [diff] [review]
> bug826753-model.patch
> 
> Review of attachment 8461239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: kickoff/model.py
> @@ +41,4 @@
> >  
> >      def __init__(self, submitter, version, buildNumber, branch,
> >                   mozillaRevision, l10nChangesets, dashboardCheck,
> > +                 mozillaRelbranch, enUSPlatforms, submittedAt=None):
> 
> I don't think enUSPlatforms can be made mandatory here. This class needs to
> be init'ed at submission time
> (http://git.mozilla.org/?p=build/release-kickoff.git;a=blob;f=kickoff/views/
> submit.py;h=5c3197bc9af6f4c2bd88b05fe5a82af4e335db88;hb=HEAD#l50), and
> enUSPlatforms won't be known at that point.

Ah yes, good catch.

> 
> @@ +190,5 @@
> >                  releases.append(r)
> >      return releases
> > +
> > +
> > +class ReleaseData(db.Model):
> 
> What do you think about calling this ReleaseEvents instead?

I am good with it, especially given future implications.

> 
> @@ +196,5 @@
> > +    """A base class with all of the common columns for any pulse.m.o update message, 
> > +    in this case from buildbot for releases."""
> > +    __tablename__ = 'release_data'
> > +    unique_name = db.Column(db.String(500), nullable=False, primary_key=True)
> > +    name = db.Column(db.String(100), nullable=False)
> 
> Why is unique_name necessary? Shouldn't name be unique already? (It is in
> the Releases' tables)

name is not unique enough. name is something like 'Firefox-31.0b5-build1', but there are 200+ rows in this table with that name. unique_name would be name+event_name... but now that I have said that, we could just make name = oldname+event_name.

> 
> @@ +256,5 @@
> > +    return statuses
> > +
> > +
> > +def getStatusTable():
> > +    return ReleaseStatus
> 
> I think getStatus and getStatusTable are both dead code now...that table is
> gone!

Yeah, yeah :) Saw this after I submitted it, but since it was feedback I figured I'd be lazy and leave it in for this feedback haha :)

Comment 20

[John Zeller [:zeller]]

Attachment: bug826753-model.patch

Comment 21

[John Zeller [:zeller]]

Are we sure that branch can *always* be broken out of the builderName? ie branch is release-mozilla-beta and builderName is release-mozilla-beta-*

If so, great! And, is the branch always 3 words? If not, is there a config list somewhere with possible branches?

Comment 22

[[e-mail] (:bhearsum)]

Comment on attachment 8461706 [details] [diff] [review]
bug826753-model.patch

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

::: kickoff/model.py
@@ +41,4 @@
>  
>      def __init__(self, submitter, version, buildNumber, branch,
>                   mozillaRevision, l10nChangesets, dashboardCheck,
> +                 mozillaRelbranch, enUSPlatforms='', submittedAt=None):

This should default to None (which ends up as NULL in the database).

It makes me wonder though: what form are these going to be stored in? Ie, if it's a string, how are you storing a list?

@@ +195,5 @@
> +
> +    """A base class to store release events primarily from buildbot."""
> +    __tablename__ = 'release_events'
> +    name = db.Column(db.String(500), nullable=False, primary_key=True)
> +    sent = db.Column(db.String(500), nullable=False)

This still needs to be a DateTime, see my previous comment for details.

@@ +196,5 @@
> +    """A base class to store release events primarily from buildbot."""
> +    __tablename__ = 'release_events'
> +    name = db.Column(db.String(500), nullable=False, primary_key=True)
> +    sent = db.Column(db.String(500), nullable=False)
> +    status_type = db.Column(db.String(500), nullable=False)

I don't think this is needed now, since you're doing everything dynamically?

@@ +233,5 @@
> +
> +def getReleaseEvents(name):
> +    events = []
> +    for r in ReleaseEvents.query.filter_by(name=name):
> +        events.append(r.toDict())

When you do re-add this, you don't need to iterate here. name is part of the PK, so it's guaranteed to be unique.

@@ +254,5 @@
> +    return events
> +
> +
> +def getReleaseEventsTable():
> +    return ReleaseEvents

This function doesn't need to exist. Consumers can import and use ReleaseEvents directly - there's no mapping to figure out, unlike with the other tables.

Comment 23

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #22)
> Comment on attachment 8461706 [details] [diff] [review]
> bug826753-model.patch
> 
> Review of attachment 8461706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: kickoff/model.py
> @@ +41,4 @@
> >  
> >      def __init__(self, submitter, version, buildNumber, branch,
> >                   mozillaRevision, l10nChangesets, dashboardCheck,
> > +                 mozillaRelbranch, enUSPlatforms='', submittedAt=None):
> 
> This should default to None (which ends up as NULL in the database).
> 
> It makes me wonder though: what form are these going to be stored in? Ie, if
> it's a string, how are you storing a list?

I am dumping them as JSON currently.

I have taken the rest of your comments into effect and submitted the model patch for review in bug 1032975 :)

Comment 24

[John Zeller [:zeller]]

Attachment: bug826753.patch

This patch represents the changing of the /releases endpoint to show a new tab that includes running releases, as well as a /releases/<releaseName>/status.html view that gives a nice looking UI to status steps.

I had a couple of outstanding questions, but I forgot them. I will update this bug when I remember :)

Let's schedule a demo so I can show you how this thing runs :)

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 8475611 [details] [diff] [review]
bug826753.patch

Rail, Sylvestre - this is going to be the basis for displaying the status that will start being submitted to Ship It soon. I'd like to see it better integrated with the existing releases.html, but I want to get other thoughts on it up front too.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

It is possible to have a demo? :)

By the way, it would be nice to remove the usage maxcdn.bootstrapcdn.com. In order to do off line development (and avoid useless tracking).

Comment 27

[[e-mail] (:bhearsum)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> It is possible to have a demo? :)
> 
> By the way, it would be nice to remove the usage maxcdn.bootstrapcdn.com. In
> order to do off line development (and avoid useless tracking).

We can probably organize one. Or maybe John can run an instance that we can all look at on our own time? I'm fine either way.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

An other instance would be fine too!

Comment 29

[John Zeller [:zeller]]

I'm absolutely okay with either! I will spin up an instance that you all can play with.

Comment 30

[[e-mail] (:bhearsum)]

(In reply to John Zeller [:zeller] from comment #29)
> I'm absolutely okay with either! I will spin up an instance that you all can
> play with.

Awesome :). I'll hold off on any feedback until that's running. dev-master1 might be a good place to run it if you haven't found one already.

Comment 31

[Rail Aliiev [:rail]]

Comment on attachment 8475611 [details] [diff] [review]
bug826753.patch

A took a look at *.py files and skimmed other files. Looks pretty straight forward.

Comment 32

[John Zeller [:zeller]]

Okay test instance is up! http://dev-master1.srv.releng.scl3.mozilla.com:5948/

Username: admin
Password: password

Comment 33

[[e-mail] (:bhearsum)]

Comment on attachment 8475611 [details] [diff] [review]
bug826753.patch

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

I haven't been able to make the web interface work yet - I think there's some issues with the code. I'm getting tracebacks from URLs like http://dev-master1.srv.releng.scl3.mozilla.com:5948/releases/Firefox-31.0-build2/status.html. I want to make sure I leave some feedback before I head out though, so I'll just be commenting on the code structure for now.

It looks good overall, I may have a couple more comments after I can view it.

::: kickoff/templates/base.html
@@ +14,4 @@
>  <script type='text/javascript' src="{{ url_for("static", filename="kickoff.js") }}"></script>
>  <!-- css -->
> +<link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/bootstrap/3.2.0/css/bootstrap.min.css">
> +<link href="//maxcdn.bootstrapcdn.com/font-awesome/4.1.0/css/font-awesome.min.css" rel="stylesheet">

Please import these into the repo. We don't want Ship It to break if that CDN is down (as unlikely as that may be).

::: kickoff/templates/includes/releases_complete.html
@@ +3,1 @@
>  {% if loop.first %}

There's probably a lot of changes we want to make to categories. I think it's probably okay to not worry about those in this initial patch. I think Sylvestre may want to take a stab at improving the overall status page once your initial work is landed (which could free you up to move on to postrelease integration!)

::: kickoff/views/status.py
@@ +75,5 @@
> +    def get(self):
> +        try:
> +            group = request.args.get('group', type=str)
> +            if group is not None:
> +                group = str(group)

args default to strings, there's no need to coerce this AFAICT. You should be able to drop the try block and use this simpler structure instead:

group = request.args.get('group') # returns None if group isn't found
if not group:
   cef_event(...)
   return ...

events = [...]
return jsonify(...)

@@ +86,5 @@
> +
> +class Status(MethodView):
> +
> +    def get(self, releaseName):
> +        status_order = ['tag', 'build', 'repack', 'update', 'releasetest', 'readyforrelease', 'postrelease']

I need to quibble with this name - nothing big though. Something like "status_groups" would be better for the list. It's a list, so adding "order" to it doesn't really mean anything - it's well known to be ordered!

Comment 34

[John Zeller [:zeller]]

Attachment: bug826753.patch

All the previous feedback has been executed. One possible issue is the introduction of some binaries for the font-awesome fonts. If there is a better way to do this, let me know.

Also the link up above should be good to go on all views now.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

(In reply to John Zeller [:zeller] from comment #32)
> http://dev-master1.srv.releng.scl3.mozilla.com:5948/
Do I need special permission to access it?

Comment 36

[John Zeller [:zeller]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #35)
> (In reply to John Zeller [:zeller] from comment #32)
> > http://dev-master1.srv.releng.scl3.mozilla.com:5948/
> Do I need special permission to access it?

Yeah creds are in comment 32

Comment 37

[[e-mail] (:bhearsum)]

Comment on attachment 8476979 [details] [diff] [review]
bug826753.patch

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

I _think_ this covers everything, but I need you to run your instance again so I can have another look at it. Perhaps it would be good to start it in a "screen" session so that it stays up even when you're offline?

::: kickoff/templates/status.html
@@ +17,3 @@
>    <div id="progress-container">
>      <h3>
> +      <strong>{{ group.title() }}</strong> 

I don't think you should be doing any manipulation of this here. The groups should be passed in in the form you want them.

::: kickoff/views/status.py
@@ +87,4 @@
>  class Status(MethodView):
>  
>      def get(self, releaseName):
> +        status_groups = ['tag', 'build', 'repack', 'update', 'releasetest', 'readyforrelease', 'postrelease']

I just noticed that these names are visible on the page. Given that, they need to be human readable. "readyforrelease" is particularly bad.

Comment 38

[[e-mail] (:bhearsum)]

Comment on attachment 8476979 [details] [diff] [review]
bug826753.patch

Okay, after looking at your instance again I'm fine with this patch as long as the couple of points from my previous comment get addressed. Very very close now!

Comment 39

[John Zeller [:zeller]]

Attachment: bug826753.patch

Made all the changes requested above. The only thing that is gonna cause trouble is the binary files for font-awesome fonts. Open to suggestions for these.

I need to regenerate this patch cause it isn't ready to apply to a fresh pull of shipit, but should be good to review.

Comment 40

[[e-mail] (:bhearsum)]

(In reply to John Zeller [:zeller] from comment #39)
> Created attachment 8481728 [details] [diff] [review]
> bug826753.patch
> 
> Made all the changes requested above. The only thing that is gonna cause
> trouble is the binary files for font-awesome fonts. Open to suggestions for
> these.

What problems will it cause? Git can handles binary files.

Comment 41

[John Zeller [:zeller]]

Attachment: bug826753.patch

Okay this has been fully tested on a new git pull from the release-kickoff repo, and works great!

One final part of this. The 'Completed' section holds all releases set to ready=1 and complete=1. Currently this includes the releases in the 'Running' section. However, the 'Running' section will only show releases that do not are not complete. I am trying to decide how we should adjust this. I'd like to have 'Completed' only show the releases after postrelease is actually finished. A possibility is that this is a small change that we do first with the post release triggering in bug 886522 (ie by consolidating the ready and complete columns into 1 status column that would be enumerated to signify ready, running, complete, etc.) What are your thoughts?

Comment 42

[John Zeller [:zeller]]

Instance is back up and updated, with the same URL and credentials as comment 32

Comment 43

[John Zeller [:zeller]]

Clarification of comment 41.

ship-it.m.o/releases.html currently has 2 tabs: Pending and Completed. Running is the one we are adding now. Since Pending releases only show up before a release is started by buildbot, and Completed *actually* means that buildbot has started, then Running effectively replaced Completed in its current form... meaning that we need to determine if we want to keep the Completed tab the way it is, and if we do, how should be make sure only truly completed releases show up there (ie releases where postrelease has been completed).

As in my email, I see 3 options:
 - Have release_runner set a new status (perhaps kickoffStatus from bug 1062187) in shipit database to signify that postrelease is done
 - Only display releases in 'Completed' that have a postrelease event in release_events table. This would require a whitelist or fake postrelease entries for the previously existing releases
 - Remove 'Completed' section altogether. After reading and working a bit on post triggering, it became apparent that we could just rename the 'Completed' section to 'Running' or 'Started' and show a status column in leiu of all 7 status columns in the current 'Running' section. Then we could slap a link over the release name that leads to the UI with progress bars for everything. 2 birds with one stone. I like this option.

What do you think?

Comment 44

[[e-mail] (:bhearsum)]

(In reply to John Zeller [:zeller] from comment #43)
> Clarification of comment 41.
> 
> ship-it.m.o/releases.html currently has 2 tabs: Pending and Completed.
> Running is the one we are adding now. Since Pending releases only show up
> before a release is started by buildbot, and Completed *actually* means that
> buildbot has started, then Running effectively replaced Completed in its
> current form... meaning that we need to determine if we want to keep the
> Completed tab the way it is, and if we do, how should be make sure only
> truly completed releases show up there (ie releases where postrelease has
> been completed).
> 
> As in my email, I see 3 options:
>  - Have release_runner set a new status (perhaps kickoffStatus from bug
> 1062187) in shipit database to signify that postrelease is done
>  - Only display releases in 'Completed' that have a postrelease event in
> release_events table. This would require a whitelist or fake postrelease
> entries for the previously existing releases
>  - Remove 'Completed' section altogether. After reading and working a bit on
> post triggering, it became apparent that we could just rename the
> 'Completed' section to 'Running' or 'Started' and show a status column in
> leiu of all 7 status columns in the current 'Running' section. Then we could
> slap a link over the release name that leads to the UI with progress bars
> for everything. 2 birds with one stone. I like this option.
> 
> What do you think?

I think the 3rd option is fine for now. We'll probably want to change things up a bit later, as we have post release tasks integrated, but there's no need to get into that just yet.

Comment 45

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #44)
> (In reply to John Zeller [:zeller] from comment #43)
> > Clarification of comment 41.
> > 
> > ship-it.m.o/releases.html currently has 2 tabs: Pending and Completed.
> > Running is the one we are adding now. Since Pending releases only show up
> > before a release is started by buildbot, and Completed *actually* means that
> > buildbot has started, then Running effectively replaced Completed in its
> > current form... meaning that we need to determine if we want to keep the
> > Completed tab the way it is, and if we do, how should be make sure only
> > truly completed releases show up there (ie releases where postrelease has
> > been completed).
> > 
> > As in my email, I see 3 options:
> >  - Have release_runner set a new status (perhaps kickoffStatus from bug
> > 1062187) in shipit database to signify that postrelease is done
> >  - Only display releases in 'Completed' that have a postrelease event in
> > release_events table. This would require a whitelist or fake postrelease
> > entries for the previously existing releases
> >  - Remove 'Completed' section altogether. After reading and working a bit on
> > post triggering, it became apparent that we could just rename the
> > 'Completed' section to 'Running' or 'Started' and show a status column in
> > leiu of all 7 status columns in the current 'Running' section. Then we could
> > slap a link over the release name that leads to the UI with progress bars
> > for everything. 2 birds with one stone. I like this option.
> > 
> > What do you think?
> 
> I think the 3rd option is fine for now. We'll probably want to change things
> up a bit later, as we have post release tasks integrated, but there's no
> need to get into that just yet.

Great I will put that all together

Comment 46

[[e-mail] (:bhearsum)]

Comment on attachment 8483151 [details] [diff] [review]
bug826753.patch

Will need a new patch for the s/Completed/Running/ thing

Comment 47

[John Zeller [:zeller]]

Attachment: bug826753.patch

Made changes to the releases.html view, so that there are now 2 tabs: Reviewed and Running/Complete. Here are a list of changes:

* New Running/Complete tab
* Added a status column to the new tab
* Added a getCurrentStatus function to the ReleaseEvents class in model.py to grab the most progressed status
* Modified the existing getReleases function in the Releases class in model.py - It called getCurrentStatus for each release listed in releases. It then sets releases.status to the return value of getCurrentStatus, if it is not None. Otherwise, it stays the same. Also 'coersing' to human readable strings here, if I may use that words to describe such a thing haha
* Modified the get function of the Status class in status.py to redirect to releases.html with errors set to warn if a release has no release events available. This is to prevent being sent to a Traceback or 404 error page when visiting old, not yet started, or completely non-existent release status pages (releases/Firefox-100.b10-build100/status.html would fail for example :) )

That is all!

Comment 48

[[e-mail] (:bhearsum)]

Comment on attachment 8487596 [details] [diff] [review]
bug826753.patch

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

95% there. Just these two things below to fix, and we can land this!

::: kickoff/model.py
@@ +291,5 @@
> +        if status:
> +            for s in status_order:
> +                if status[s]['progress'] == 0:
> +                    return currentStatus
> +                currentStatus = s

This reads like currentStatus gets set to the most recent status, and then you return it when you see the next status has not completed. I think this would be much clearer if you iterated over status_order backwards -- then you could just check for "if status[s]["progress"]" and return, rather than waiting for the next iteration.

::: kickoff/views/status.py
@@ +93,5 @@
> +        errors = {}
> +        if not status:
> +            errors = ['No release events found for {}'.format(releaseName)]
> +            form = ReleasesForm()
> +            return render_template('releases.html', releases=sortedReleases(), form=form, errors=errors)

This is a weird redirection. It makes sense if you're accessing one of these status pages from releases.html, but it's strange if someone tries to manually edit an URL or something. I think the error case should use status.html, too, even if it just shows nothing except the error message.

Comment 49

[John Zeller [:zeller]]

Attachment: add status page to ship it

I have made the changes you requested! And the test instance is updated too: http://dev-master1.srv.releng.scl3.mozilla.com:5948

Comment 50

[[e-mail] (:bhearsum)]

Comment on attachment 8488339 [details] [diff] [review]
add status page to ship it

I landed this and pushed it to stage and prod. It's pretty much a no-op for now, until bugs 1055191 and 1032978 are fixed. When they land I'll be sending out some mail to give people a heads up about what to expect from the new and improved Ship It.

Comment 51

[Lawrence Mandel [:lmandel] (use needinfo)]

I'd really like to see this functionality in ship-it. I think this would be a very useful addition for relman. With bug 1055191 resolved it looks like this is only blocked on bug 1032978. Can this work be added to the Q4 list?

Comment 52

[John Zeller [:zeller]]

(In reply to Lawrence Mandel [:lmandel] from comment #51)
> I'd really like to see this functionality in ship-it. I think this would be
> a very useful addition for relman. With bug 1055191 resolved it looks like
> this is only blocked on bug 1032978. Can this work be added to the Q4 list?

Ran into a couple of bugs on this, so it should be absolutely doable for Q4, if not within the next couple of weeks.

Comment 53

[Rail Aliiev [:rail]]

release promotion FTW!