Upload retry summary to desired location instead of uploading then moving.

Upload retry summary to desired location instead of uploading then moving.

In the first version of archive_layout_test_retry_summary.py,
the temp file is uploaded to GS and then moved in GS, but
this doesn't work how I expected; the temp file is left around
(e.g. gs://chromium-layout-test-archives/linux_precise_blink_rel/4185)

Instead of uploading then moving, this CL changes slave_utils.GSUtilCopyfile to allow specifying a destination filename.

BUG=642980
Committed: https://chromium.googlesource.com/chromium/tools/build/+/a385da971c2908ba4501b5af3a1e54274f4b25a8

[qyearsley, 2016-10-21 17:21:56]

qyearsley@chromium.org changed reviewers:
+ dpranke@chromium.org, phajdan.jr@chromium.org

[qyearsley, 2016-10-21 17:21:57]

This CL isn't urgent.

Dirk, does this seem OK overall?

Pawel, does this seem like an appropriate use of robust_tempdir? (or do you
think that using a temp dir is unnecessary in this case?)

Thanks :-)

[Paweł Hajdan Jr., 2016-10-24 12:07:18]

https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
File scripts/slave/chromium/archive_layout_test_retry_summary.py (right):

https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
scripts/slave/chromium/archive_layout_test_retry_summary.py:34: temp_path =
os.path.join(temp_dir, 'retry_summary.json')
I'm wondering whether this works.

Could you explicitly call cleanup and tempdir methods of temp_dir? See how it's
used in remote_run.py .

As an alternative, if you're not using cleanup(), RobustTempdir probably doesn't
give you any advantage over plain tempfile.

https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
scripts/slave/chromium/archive_layout_test_retry_summary.py:36:
slave_utils.GSUtilCopyFile(temp_path, gs_base,
Why do we copy to temporary directory for this?

It seems one could call GSUtilCopyFile right with args.retry_summary_json, and
make sure the destination on the GS side matches what we want.

[qyearsley, 2016-10-24 16:48:48]

https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
File scripts/slave/chromium/archive_layout_test_retry_summary.py (right):

https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
scripts/slave/chromium/archive_layout_test_retry_summary.py:34: temp_path =
os.path.join(temp_dir, 'retry_summary.json')
On 2016/10/24 at 12:07:18, Paweł Hajdan Jr. wrote:
> I'm wondering whether this works.
> 
> Could you explicitly call cleanup and tempdir methods of temp_dir? See how
it's used in remote_run.py .
> 
> As an alternative, if you're not using cleanup(), RobustTempdir probably
doesn't give you any advantage over plain tempfile.

Oh, I see -- robust_tempdir.RobustTempdir() gives you a base dir, and then the
normal usage is to clean up and make temp dirs inside of that base dir.

Makes sense, thanks.

https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
scripts/slave/chromium/archive_layout_test_retry_summary.py:36:
slave_utils.GSUtilCopyFile(temp_path, gs_base,
On 2016/10/24 at 12:07:18, Paweł Hajdan Jr. wrote:
> Why do we copy to temporary directory for this?
> 
> It seems one could call GSUtilCopyFile right with args.retry_summary_json, and
make sure the destination on the GS side matches what we want.

The reason for this is that slave_utils.GSUtilCopyFile as-is doesn't let one
specify destination file name -- destination file name is the same was whatever
filename was passed in.

But I suppose this is an artificial limitation, and maybe it's OK to modify
slave_utils.GSUtilCopyFile to optionally take a destination file name?

[qyearsley, 2016-10-24 17:24:25]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 17:24:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qyearsley, 2016-10-24 17:27:14]

On 2016/10/24 at 16:48:48, qyearsley wrote:
>
https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
> File scripts/slave/chromium/archive_layout_test_retry_summary.py (right):
> 
>
https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
> scripts/slave/chromium/archive_layout_test_retry_summary.py:34: temp_path =
os.path.join(temp_dir, 'retry_summary.json')
> On 2016/10/24 at 12:07:18, Paweł Hajdan Jr. wrote:
> > I'm wondering whether this works.
> > 
> > Could you explicitly call cleanup and tempdir methods of temp_dir? See how
it's used in remote_run.py .
> > 
> > As an alternative, if you're not using cleanup(), RobustTempdir probably
doesn't give you any advantage over plain tempfile.
> 
> Oh, I see -- robust_tempdir.RobustTempdir() gives you a base dir, and then the
normal usage is to clean up and make temp dirs inside of that base dir.
> 
> Makes sense, thanks.
> 
>
https://codereview.chromium.org/2442693003/diff/1/scripts/slave/chromium/arch...
> scripts/slave/chromium/archive_layout_test_retry_summary.py:36:
slave_utils.GSUtilCopyFile(temp_path, gs_base,
> On 2016/10/24 at 12:07:18, Paweł Hajdan Jr. wrote:
> > Why do we copy to temporary directory for this?
> > 
> > It seems one could call GSUtilCopyFile right with args.retry_summary_json,
and make sure the destination on the GS side matches what we want.
> 
> The reason for this is that slave_utils.GSUtilCopyFile as-is doesn't let one
specify destination file name -- destination file name is the same was whatever
filename was passed in.
> 
> But I suppose this is an artificial limitation, and maybe it's OK to modify
slave_utils.GSUtilCopyFile to optionally take a destination file name?

Now changed this CL to modify slave_utils.GSUtilCopyFile to take a dest_filename
argument.

[commit-bot: I haz the power, 2016-10-24 17:35:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-24 17:35:50]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-10-24 19:10:21]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 19:10:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-24 19:22:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-24 19:22:20]

Dry run: This issue passed the CQ dry run.

[qyearsley, 2016-10-25 20:15:13]

Description was changed from

==========
When archiving layout test retry summary, rename locally before upload.

In the first version of archive_layout_test_retry_summary.py,
the temp file is uploaded to GS and then moved in GS, but
this doesn't work how I expected; the temp file is left around
(e.g. gs://chromium-layout-test-archives/linux_precise_blink_rel/4185)

Instead of uploading then moving, this CL changes it so that the script
moves the file locally then uploads.

BUG=642980
==========

to

==========
Upload retry summary to desired location instead of uploading then moving.

In the first version of archive_layout_test_retry_summary.py,
the temp file is uploaded to GS and then moved in GS, but
this doesn't work how I expected; the temp file is left around
(e.g. gs://chromium-layout-test-archives/linux_precise_blink_rel/4185)

Instead of uploading then moving, this CL changes slave_utils.GSUtilCopyfile to
allow specifying a destination filename.

BUG=642980
==========

[qyearsley, 2016-10-25 20:15:26]

Pawel, do you think this looks OK now?

[Paweł Hajdan Jr., 2016-10-26 06:30:03]

LGTM

[qyearsley, 2016-10-26 15:38:16]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2016-10-26 15:38:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 15:48:33]

Description was changed from

==========
Upload retry summary to desired location instead of uploading then moving.

In the first version of archive_layout_test_retry_summary.py,
the temp file is uploaded to GS and then moved in GS, but
this doesn't work how I expected; the temp file is left around
(e.g. gs://chromium-layout-test-archives/linux_precise_blink_rel/4185)

Instead of uploading then moving, this CL changes slave_utils.GSUtilCopyfile to
allow specifying a destination filename.

BUG=642980
==========

to

==========
Upload retry summary to desired location instead of uploading then moving.

In the first version of archive_layout_test_retry_summary.py,
the temp file is uploaded to GS and then moved in GS, but
this doesn't work how I expected; the temp file is left around
(e.g. gs://chromium-layout-test-archives/linux_precise_blink_rel/4185)

Instead of uploading then moving, this CL changes slave_utils.GSUtilCopyfile to
allow specifying a destination filename.

BUG=642980

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/a385da971c2908ba4501...
==========

[commit-bot: I haz the power, 2016-10-26 15:48:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/tools/build/+/a385da971c2908ba4501...

[Dirk Pranke, 2016-10-26 18:59:14]

lgtm