config_service: Changed file links to point to latest revision instead of master

config_service: Changed file links to point to latest revision instead of master

Config service now uses information from latest accepted revision to direct user
to the correct link

BUG=749534
Review-Url: https://codereview.chromium.org/2985223002
Committed: https://github.com/luci/luci-py/commit/5925b54b4ff3522c85710eda81d4305074663fef

[cwpayton, 2017-07-27 18:52:34]

cwpayton@google.com changed reviewers:
+ hinoka@chromium.org, nodir@chromium.org, sergeyberezin@chromium.org

[cwpayton, 2017-07-27 18:52:35]

PTAL

[Sergey Berezin, 2017-07-27 21:14:14]

Overall looks good, just a couple of comments. Thanks!

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set.html (right):

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set.html:264: } else if
(config_set.revision && config_set.revision.url) {
Is config_set.revision_url always the same as last_import_attempt.revision_url
if the last attempt was successful? If so, maybe the first condition isn't
necessary?

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:56: url:
"https://test.com/valid-project-with-config-files"
Make this URL different from `location`, so you can test which one gets picked.
Also, please add tests for each branch of the if-condition (when revision
exists, and when it doesn't), for better coverage.

Here you may benefit from a helper function to set up a server with a
customizable response, e.g. see
https://codereview.chromium.org/2990713002/diff/140001/appengine/config_servi...

[cwpayton, 2017-07-27 22:56:04]

PTAL

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/src/config-ui/config-set.html (right):

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
appengine/config_service/ui/src/config-ui/config-set.html:264: } else if
(config_set.revision && config_set.revision.url) {
On 2017/07/27 21:14:14, Sergey Berezin wrote:
> Is config_set.revision_url always the same as last_import_attempt.revision_url
> if the last attempt was successful? If so, maybe the first condition isn't
> necessary?

Done.

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2985223002/diff/20001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:56: url:
"https://test.com/valid-project-with-config-files"
On 2017/07/27 21:14:14, Sergey Berezin wrote:
> Make this URL different from `location`, so you can test which one gets
picked.
> Also, please add tests for each branch of the if-condition (when revision
> exists, and when it doesn't), for better coverage.
> 
> Here you may benefit from a helper function to set up a server with a
> customizable response, e.g. see
>
https://codereview.chromium.org/2990713002/diff/140001/appengine/config_servi...

Done.

[Sergey Berezin, 2017-07-27 23:03:58]

LGTM + 2 tiny nits. Thanks!

https://codereview.chromium.org/2985223002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2985223002/diff/40001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:101:
test('correct url when revision is provided', function(done) {
nit: '_has_ correct url...'

https://codereview.chromium.org/2985223002/diff/40001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:128:
test('correct url when revision is not provided', function(done) {
nit: 'has correct url...'

[cwpayton, 2017-07-27 23:14:26]

https://codereview.chromium.org/2985223002/diff/40001/appengine/config_servic...
File appengine/config_service/ui/test/config-ui/config-set_test.html (right):

https://codereview.chromium.org/2985223002/diff/40001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:101:
test('correct url when revision is provided', function(done) {
On 2017/07/27 23:03:58, Sergey Berezin wrote:
> nit: '_has_ correct url...'

Done.

https://codereview.chromium.org/2985223002/diff/40001/appengine/config_servic...
appengine/config_service/ui/test/config-ui/config-set_test.html:128:
test('correct url when revision is not provided', function(done) {
On 2017/07/27 23:03:58, Sergey Berezin wrote:
> nit: 'has correct url...'

Done.

[cwpayton, 2017-07-27 23:14:54]

The CQ bit was checked by cwpayton@google.com

[cwpayton, 2017-07-27 23:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyberezin@chromium.org
Link to the patchset: https://codereview.chromium.org/2985223002/#ps60001
(title: "Fixed nit on test label")

[commit-bot: I haz the power, 2017-07-27 23:14:59]

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-07-27 23:17:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501197294013980,
"parent_rev": "d3bd7823b4fe248ccc7e513c1039b36e00d20dad", "commit_rev":
"5925b54b4ff3522c85710eda81d4305074663fef"}

[commit-bot: I haz the power, 2017-07-27 23:17:33]

Description was changed from

==========
config_service: Changed file links to point to latest revision instead of master

Config service now uses information from latest accepted revision to direct user
to the correct link

BUG=749534
==========

to

==========
config_service: Changed file links to point to latest revision instead of master

Config service now uses information from latest accepted revision to direct user
to the correct link

BUG=749534

Review-Url: https://codereview.chromium.org/2985223002
Committed:
https://github.com/luci/luci-py/commit/5925b54b4ff3522c85710eda81d4305074663fef
==========

[commit-bot: I haz the power, 2017-07-27 23:17:34]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-py/commit/5925b54b4ff3522c85710eda81d4305074663fef