Support network service apptests in the upload_service script

Support network service apptests in the upload_service script

BUG=450356
R=blundell@chromium.org, jamesr@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/bf9a4b53049b10cfdd85934075ed3e147be889f5

[ppi, 2015-02-04 19:46:35]

Hi James,

ptal.

[jamesr, 2015-02-04 20:14:39]

What is the path component changing from and why?

[ppi, 2015-02-04 20:27:15]

The path used to be:

"gs://mojo/SERVICE/VERSION/PLATFORM"

The change is to add /BINARY_NAME, so that you have different paths for
network_service and network_service_apptests .

[jamesr, 2015-02-04 20:58:58]

What's the distinction between "service" and "binary_name", then?  I would think
network_service.mojo and network_service_apptests.mojom would be peers since
they are both mojo applications.

[ppi, 2015-02-04 21:03:15]

tal at the code - as it is right now, service is an identifier mapping to one or
more binaries that are uploaded together.

I went this way to ensure that one uploads both network_service and
network_service_apptests when running "upload_service network".

We can go another way if you feel strongly about that.

[ppi, 2015-02-05 09:07:52]

ppi@chromium.org changed reviewers:
+ blundell@chromium.org

[ppi, 2015-02-05 09:07:53]

+ Colin, ptal.

[blundell, 2015-02-05 10:53:56]

https://codereview.chromium.org/904443003/diff/1/mojo/services/upload_service.py
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/1/mojo/services/upload_service...
mojo/services/upload_service.py:17: SERVICES = [ "html_viewer", "network" ]
I suggest changing this to the binary name (i.e., network_service) and adding a
separate list of services with associated apps, with a comment that "associated
apps" are ones that should be versioned in lockstep with their primary apps.

[ppi, 2015-02-05 12:35:06]

Thanks, Colin, ptal.

https://codereview.chromium.org/904443003/diff/1/mojo/services/upload_service.py
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/1/mojo/services/upload_service...
mojo/services/upload_service.py:17: SERVICES = [ "html_viewer", "network" ]
On 2015/02/05 10:53:56, blundell wrote:
> I suggest changing this to the binary name (i.e., network_service) and adding
a
> separate list of services with associated apps, with a comment that
"associated
> apps" are ones that should be versioned in lockstep with their primary apps.

Done.

[blundell, 2015-02-05 12:45:50]

https://codereview.chromium.org/904443003/diff/20001/mojo/services/upload_ser...
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/20001/mojo/services/upload_ser...
mojo/services/upload_service.py:25: ASSOCIATED_SERVICES = {
As discussed offline, I think we should consider these as apps that are
associated with services rather than "services" themselves.

[ppi, 2015-02-05 12:53:57]

Patchset #3 (id:40001) has been deleted

[ppi, 2015-02-05 12:55:37]

Thanks, ptal.

https://codereview.chromium.org/904443003/diff/20001/mojo/services/upload_ser...
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/20001/mojo/services/upload_ser...
mojo/services/upload_service.py:25: ASSOCIATED_SERVICES = {
On 2015/02/05 12:45:50, blundell wrote:
> As discussed offline, I think we should consider these as apps that are
> associated with services rather than "services" themselves.

Done.

[blundell, 2015-02-05 13:03:22]

lgtm

You should have the changes to the download script in the Mojo repo ready to
land before landing this CL.

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
mojo/services/upload_service.py:17: # (network_service_apptests.mojo) and a zip
with the service mojoms.
mojos if any.

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
mojo/services/upload_service.py:89: def upload_service(service, binary_dir,
platform, dry_run):
s/upload_service/upload_apps? I would expect a function called 'upload_service'
to also upload the mojoms.

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
mojo/services/upload_service.py:145: "linux-x64", args.dry_run)
nit: indentation, here and in the corresponding line below.

[ppi, 2015-02-05 13:11:21]

New patchsets have been uploaded after l-g-t-m from blundell@chromium.org

[ppi, 2015-02-05 13:29:07]

Thanks, Colin. The Mojo side is at https://codereview.chromium.org/879243006/ .

James, does this look good now?

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
mojo/services/upload_service.py:17: # (network_service_apptests.mojo) and a zip
with the service mojoms.
On 2015/02/05 13:03:22, blundell wrote:
> mojos if any.

Done.

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
mojo/services/upload_service.py:89: def upload_service(service, binary_dir,
platform, dry_run):
On 2015/02/05 13:03:22, blundell wrote:
> s/upload_service/upload_apps? I would expect a function called
'upload_service'
> to also upload the mojoms.

Done.

https://codereview.chromium.org/904443003/diff/60001/mojo/services/upload_ser...
mojo/services/upload_service.py:145: "linux-x64", args.dry_run)
On 2015/02/05 13:03:22, blundell wrote:
> nit: indentation, here and in the corresponding line below.

Done.

[ppi, 2015-02-05 23:00:39]

James, could you take a look?

[jamesr, 2015-02-05 23:41:03]

On 2015/02/04 21:03:15, ppi wrote:
> tal at the code - as it is right now, service is an identifier mapping to one
or
> more binaries that are uploaded together.

This doesn't make sense to me.

> 
> I went this way to ensure that one uploads both network_service and
> network_service_apptests when running "upload_service network".
> 
> We can go another way if you feel strongly about that.

network_service.mojo and network_apptests.mojo are both mojo apps (peers in our
taxonomy) identifiable by URLs.  Adding an additional hierarchy layer in the URL
scheme adds overhead with no real benefit.

[jamesr, 2015-02-05 23:42:31]

Hmm, the patch description makes it seem like it's not in the URL scheme at all.
 Which is it?  If you just want a list of which .mojos to upload then I'd
suggest finding a less overloaded term than "service" to specify which it is, or
get rid of that idea and just list out the things you want.

[ppi, 2015-02-05 23:52:21]

As you and Colin suggested, network_service and network_service_apptests are now
peers in the url hierarchy.

Not sure if I follow the last remark - as far as the script is concerned, one
can ask to upload either network_service or html_viewer. Uploading
network_service also uploads the apptests, but this is a detail. We want the
apptests available in GS whenever the service is there, so it's easier to upload
them together.

Or do you mean that the script should not be called upload_service?

Wdyt?

[jamesr, 2015-02-06 00:18:03]

On 2015/02/05 23:52:21, ppi wrote:
> As you and Colin suggested, network_service and network_service_apptests are
now
> peers in the url hierarchy.
> 

OK cool - sorry for missing this (i'm not 100% right now)

> Not sure if I follow the last remark - as far as the script is concerned, one
> can ask to upload either network_service or html_viewer. Uploading
> network_service also uploads the apptests, but this is a detail. We want the
> apptests available in GS whenever the service is there, so it's easier to
upload
> them together.
> 
> Or do you mean that the script should not be called upload_service?
> 
> Wdyt?

I would suggest either having the script only know about the set of .mojos it
knows about and deferring decisions about which are considered 'related' or not
to the caller (presumably a bot step) or getting rid of the configuration
completely and having it always upload a list of .mojos.  I think trying to
categorize things overly here is just going to lead to confusion.  The set of
apps that different things will need will evolve over time and depend on the use
case.  Our system is set up such that if the set of potentially useful things
are all available at known URLs then the system can find and run them as needed.

[blundell, 2015-02-06 09:48:07]

On 2015/02/06 00:18:03, jamesr wrote:
> On 2015/02/05 23:52:21, ppi wrote:
> > As you and Colin suggested, network_service and network_service_apptests are
> now
> > peers in the url hierarchy.
> > 
> 
> OK cool - sorry for missing this (i'm not 100% right now)
> 
> > Not sure if I follow the last remark - as far as the script is concerned,
one
> > can ask to upload either network_service or html_viewer. Uploading
> > network_service also uploads the apptests, but this is a detail. We want the
> > apptests available in GS whenever the service is there, so it's easier to
> upload
> > them together.
> > 
> > Or do you mean that the script should not be called upload_service?
> > 
> > Wdyt?
> 
> I would suggest either having the script only know about the set of .mojos it
> knows about and deferring decisions about which are considered 'related' or
not
> to the caller (presumably a bot step) or getting rid of the configuration
> completely and having it always upload a list of .mojos.  I think trying to
> categorize things overly here is just going to lead to confusion.  The set of
> apps that different things will need will evolve over time and depend on the
use
> case.  Our system is set up such that if the set of potentially useful things
> are all available at known URLs then the system can find and run them as
needed.

The primary concern that ppi@ has is the likelihood that a developer uploads a
new version of network_service.mojo but forgets to upload a new version of
network_service_apptests.mojo. We could square the circle here by making
upload_service.py "dumb" as James is suggesting (so that it just knows how to
upload one app from its list of apps), and adding an upload_network_service.py
wrapper that makes the necessary invocations to ensure that everything is
properly uploaded. Uploading the entire list of .mojos would also make sense as
long as we are confident that there is enough vetting of these .mojos in the
Chromium waterfall that we would be uploading trustworthy ones by virtue of the
Chromium waterfall having been green (compared to developers making explicit
decisions on when to rev the impl of a given service, which seems to be the
model we're at now with these services).

[ppi, 2015-02-06 12:45:06]

Patchset #5 (id:100001) has been deleted

[ppi, 2015-02-06 12:46:32]

As Colin said, my motivation was to provide this sanitization of uploading the
service and the tests together, but I don't mind keeping things simple. We
should automate the upload anyway.

In patch set 5 I drop any association between apps from the script.

Colin, James - ptal.

[blundell, 2015-02-06 13:10:46]

lgtm

https://codereview.chromium.org/904443003/diff/120001/mojo/services/upload_se...
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/120001/mojo/services/upload_se...
mojo/services/upload_service.py:82: dest = dest_dir + binary_name + ".zip"
nit: You can just specify dest with the "+ binary name" at line 71, and then add
the ".zip" extension here.

[ppi, 2015-02-06 13:16:35]

Thanks!

https://codereview.chromium.org/904443003/diff/120001/mojo/services/upload_se...
File mojo/services/upload_service.py (right):

https://codereview.chromium.org/904443003/diff/120001/mojo/services/upload_se...
mojo/services/upload_service.py:82: dest = dest_dir + binary_name + ".zip"
On 2015/02/06 13:10:44, blundell wrote:
> nit: You can just specify dest with the "+ binary name" at line 71, and then
add
> the ".zip" extension here.

Unless you feel strongly, I'd prefer not to have "dest" mean full and actual
destination for not zipped binaries and only a prefix of the destination path
for zipped binaries. I like that dest_dir means the same in both cases.

[blundell, 2015-02-06 13:17:08]

lgtm

[jamesr, 2015-02-06 17:47:49]

lgtm

[ppi, 2015-02-09 08:44:07]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2015-02-09 08:44:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/904443003/120001

[commit-bot: I haz the power, 2015-02-09 10:17:32]

Patchset 5 (id:??) landed as
https://crrev.com/bf9a4b53049b10cfdd85934075ed3e147be889f5
Cr-Commit-Position: refs/heads/master@{#315278}

[ppi, 2015-02-09 10:17:41]

Committed patchset #5 (id:120001) manually as
bf9a4b53049b10cfdd85934075ed3e147be889f5 (presubmit successful).