[wpr-go] Add support install test root CA on Android

[wpr-go] Add support install test root CA on Android

This CL adds support for installing a test root CA on Android.

BUG=catapult:#3735

Review-Url: https://codereview.chromium.org/2997573003
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/ffd86f1e6c9de3b455b68af1d6dc1e15fefdfe5a

[xunjieli, 2017-08-09 14:33:14]

Patchset #2 (id:20001) has been deleted

[xunjieli, 2017-08-09 14:33:21]

Patchset #1 (id:1) has been deleted

[xunjieli, 2017-08-09 14:35:04]

xunjieli@chromium.org changed reviewers:
+ nednguyen@google.com, tombergan@chromium.org

[xunjieli, 2017-08-09 14:35:04]

PTAL. Thank you. This works on my Nexus5x running M. I am yet to test on other
configurations.

[xunjieli, 2017-08-09 14:35:09]

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

[commit-bot: I haz the power, 2017-08-09 14:35:12]

Dry run: CQ is trying da patch.

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

[xunjieli, 2017-08-09 14:35:13]

The CQ bit was unchecked by xunjieli@chromium.org

[nednguyen, 2017-08-09 14:39:15]

High level comments:
1) We need to know the device id for this command to work: a host computer can
have multiple android devices plugged in.
2) We should also allow users to specify which adb binary path to use for this
command, since different adb binary can behave differently (Telemetry uses it
own copy of adb binary instead of relying on user's existing one)

[Tom Bergan, 2017-08-09 15:20:51]

lgtm

I don't know much about adb so I'll defer to Ned on those specifics. The Go code
looks good.

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
File web_page_replay_go/src/webpagereplay/adb_cert_installer.go (right):

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/adb_cert_installer.go:29: fmt.Println(args)
Suggest cmd.Args here, as that will include "adb".

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/adb_cert_installer.go:73: certPath =
"newcert.pem"
Is this temporary code?

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
File web_page_replay_go/src/webpagereplay/installroot.go (right):

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/installroot.go:29: if isAndroid {
Do you want to do this before the runtime.GOOS check? (If AdbInstallRoot only
works on linux, I'd add another runtime.GOOS check there.)

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/installroot.go:74: if isAndroid {
ditto here

[xunjieli, 2017-08-09 15:51:32]

Thank you for the review. I have addressed all comments. PTAL.

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
File web_page_replay_go/src/webpagereplay/adb_cert_installer.go (right):

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/adb_cert_installer.go:29: fmt.Println(args)
On 2017/08/09 15:20:50, Tom Bergan wrote:
> Suggest cmd.Args here, as that will include "adb".

Done. Good idea! This is much more informative.

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/adb_cert_installer.go:73: certPath =
"newcert.pem"
On 2017/08/09 15:20:50, Tom Bergan wrote:
> Is this temporary code?

Done. Good catch!

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
File web_page_replay_go/src/webpagereplay/installroot.go (right):

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/installroot.go:29: if isAndroid {
On 2017/08/09 15:20:51, Tom Bergan wrote:
> Do you want to do this before the runtime.GOOS check? (If AdbInstallRoot only
> works on linux, I'd add another runtime.GOOS check there.)

Done. I added a check here.

https://codereview.chromium.org/2997573003/diff/40001/web_page_replay_go/src/...
web_page_replay_go/src/webpagereplay/installroot.go:74: if isAndroid {
On 2017/08/09 15:20:50, Tom Bergan wrote:
> ditto here

Done.

[nednguyen, 2017-08-09 16:48:13]

lgtm

[xunjieli, 2017-08-09 16:56:26]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2017-08-09 16:56:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
tombergan@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/2997573003/#ps120001
(title: "roll binary")

[commit-bot: I haz the power, 2017-08-09 16:56:33]

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, 2017-08-09 17:23:46]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1502297786023700,
"parent_rev": "2e43e3c5cd3c41133a65a606ec3e7ae22a655ca6", "commit_rev":
"ffd86f1e6c9de3b455b68af1d6dc1e15fefdfe5a"}

[commit-bot: I haz the power, 2017-08-09 17:23:49]

Description was changed from

==========
[wpr-go] Add support install test root CA on Android

This CL adds support for installing a test root CA on Android.

BUG=catapult:#3735
==========

to

==========
[wpr-go] Add support install test root CA on Android

This CL adds support for installing a test root CA on Android.

BUG=catapult:#3735

Review-Url: https://codereview.chromium.org/2997573003
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-08-09 17:23:50]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...