New issue
Advanced search Search tips

Issue 747645 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Fix links to config files with scheduler configurations

Project Member Reported by vadimsh@chromium.org, Jul 22 2017

Issue description

LUCI Scheduler fetches configurations files (with job definitions) via luci-config. As a convenience feature, it also displays links to original config files in its UI.

For example, on this page https://luci-scheduler.appspot.com/jobs/infra/infra-continuous-trusty-64 the link is "Definition (rev <LINK>)": https://screenshot.googleplex.com/fpFvczBSnd4.png

The scheduler currently does that by "guessing" the link based on other properties of the config file: https://cs.chromium.org/chromium/infra/go/src/github.com/luci/luci-go/scheduler/appengine/catalog/catalog.go?q=getRevisionURL&sq=package:chromium&l=318

This guessing is generally wrong. For example, it doesn't work with this job: https://luci-scheduler.appspot.com/jobs/fuchsia/clang-x86_64-linux 

It works only with projects that follow standard convention for storing config files ("refs/heads/config" ref). Fuchsia project doesn't follow it.

Instead, the scheduler should use revision.url field returned as part of luci-config response (or maybe "location"?.. Both seem ~= appropriate. If we use 'location' we should manually replace the symbolic ref with concrete revision. Seems revision.url has this done already). 

E.g. https://apis-explorer.appspot.com/apis-explorer/?base=https://luci-config.appspot.com/_ah/api#p/config/v1/config.get_config_sets?config_set=projects%252Ffuchsia&_h=3& (which is https://screenshot.googleplex.com/O34Sj7XBcwR.png).

So we need to update luci-config client in luci-go to expose this information, and the make use of it in luci-scheduler.

luci-config client, unfortunately, is quite complicated... I'm not sure what exactly we'll need to change there. As starting point, looks like we need to add new 'ViewURL' field (or something like that) to this struct https://github.com/luci/luci-go/blob/02528b08305c7b4bd442e5cf7ce9c5d3e8324f91/luci_config/server/cfgclient/config.go#L35

(and then figure out how to correctly populate it, considering all the caching layers in luci-config go client).
 
Few more general instructions, since this is your first luci-go task, and IIUC, you are not familiar with our go environment.

There are multiple ways to get and build luci-go code. Simplest is to use infra.git's hermetic Go environment (same as used by the bots to run tests) [1]:

1. Checkout infra.git gclient solution (if you don't have it already): "fetch infra". ('fetch' is part of depot_tools).
2. This solution contains luci-go inside of it, pinned at some good revision: https://chromium.googlesource.com/infra/infra/+/master/DEPS#25 This is the revision used to build Go code that is located directly in infra.git repo.
3. Since we are want to develop luci-go at HEAD, we need to checkout latest revision:

cd <solution_root>/infra/go/src/github.com/luci/luci-go/
git fetch origin
git checkout origin/master

4. Next we will need to switch console into the infra's go environment. It contains go toolset and a bunch of other executables (like protocol buffer compiler): 

cd <solution_root>/infra/go/
eval `./env.py`

(env.py is a script that installs and updates everything. It spits out shell script that modifies PATH and other environment variable. By eval executes this script directly inside current shell, thus updating PATH).

5. To confirm:

go version
> go version go1.8.3 darwin/amd64

6. Now the current shell has perfectly capable Go toolset (with configured PATH, GOROOT and GOPATH). The root of the Go workspace is '<solution_root>/infra/go/'. In particular, all source code is in '<solution_root>/infra/go/src'. All go commands accept package paths relative to this root.

7. For example, to run test for all LUCI Scheduler code:

go test github.com/luci/luci-go/scheduler/...

8. Same way of doing it:

cd <solution_root>/infra/go/src/github.com/luci/luci-go/scheduler/
go test ./...

9. Making new CL and uploading it for review is standard depot_tools flow:

git new-branch my_feature
...
git cl upload

(We still use Rietveld, it will upload to Rietveld).
(If you are not familiar with 'new-branch' there's excellent depot_tools tutorial [2]).

10. You can run LUCI Scheduler GAE app locally using 'gae.py' tool (it's a wrapper around GAE SDK, it's in PATH due to eval `./env.py` call in step 4):

cd .../luci-go/scheduler/appengine
gae.py devserver

// Then visit http://localhost:8080/ in the browser (it is a bit slow on first request, since it compiles all source code).

11. Deploying is also done through gae.py:

cd .../luci-go/scheduler/appengine
gae.py upload -A luci-scheduler-dev

(You may not have permissions to do so, though).

----

Links:
[1] https://chromium.googlesource.com/infra/infra/+/master/go/README.md
[2] https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/depot_tools_tutorial.html
Cc: d...@chromium.org vadimsh@chromium.org
+Dan

Thanks Vadim!

I've reuploaded the two potential CLs to update for the Gerrit migration:
https://chromium-review.googlesource.com/c/608753 (augment config metas)
https://chromium-review.googlesource.com/c/608757 (replace location URL with struct);
PTAL and let me know what you think.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/c6fcabf20dcbd78205ccc6edd70286f2b39fcdf0

commit c6fcabf20dcbd78205ccc6edd70286f2b39fcdf0
Author: Jao-ke Chin-Lee <jchinlee@chromium.org>
Date: Thu Aug 10 23:52:58 2017

Provide latest revision url in get_[multi_]configs.

If the latest config is requested, both latest revision and latest revision URL
(as ViewURL) are populated in the response. If a config is requested given
revision, no revision URL is provided.

Bug:  747645 
Change-Id: If0a59f96f0342e6e6dd20c1279464e79f343f000
Reviewed-on: https://chromium-review.googlesource.com/611210
Commit-Queue: Jao-ke Chin-Lee <jchinlee@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/c6fcabf20dcbd78205ccc6edd70286f2b39fcdf0/appengine/config_service/api.py
[modify] https://crrev.com/c6fcabf20dcbd78205ccc6edd70286f2b39fcdf0/appengine/config_service/api_test.py
[modify] https://crrev.com/c6fcabf20dcbd78205ccc6edd70286f2b39fcdf0/appengine/config_service/storage.py
[modify] https://crrev.com/c6fcabf20dcbd78205ccc6edd70286f2b39fcdf0/appengine/config_service/storage_test.py

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/2c80f717f055fcd29797110bf52852deca47554e

commit 2c80f717f055fcd29797110bf52852deca47554e
Author: Jao-ke Chin-Lee <jchinlee@chromium.org>
Date: Fri Aug 11 20:14:09 2017

Add ViewURL to config metas.

Currently, the frontend displays a config URL that is constructed using config
location, revision, and config path, and a guess as to the structure. This guess
is not alway correct.

This CL augments the API with a config URL for the frontend to display directly
instead.

Bug:  747645 
Change-Id: I1bbfcc8d052441b64d314ccd33715c9b69602262
Reviewed-on: https://chromium-review.googlesource.com/608753
Commit-Queue: Jao-ke Chin-Lee <jchinlee@chromium.org>
Reviewed-by: Daniel Jacques <dnj@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/api/luci_config/config/v1/config-api.json
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/api/luci_config/config/v1/config-gen.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/config/impl/filesystem/fs.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/config/impl/filesystem/fs_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/config/impl/remote/remote.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/config/impl/remote/remote_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/common/config/interface.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/appengine/backend/datastore/ds_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/appengine/gaeconfig/default_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/caching/config.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/caching/config_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/client/client.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/client/client_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/format/format_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/meta.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/backend/testconfig/local_service_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/config.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/config_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/luci_config/server/cfgclient/textproto/resolver_test.go
[modify] https://crrev.com/2c80f717f055fcd29797110bf52852deca47554e/scheduler/appengine/catalog/catalog.go

Project Member

Comment 6 by bugdroid1@chromium.org, Aug 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/7a434b550599df6ea1425eb7789ebc6261d9056c

commit 7a434b550599df6ea1425eb7789ebc6261d9056c
Author: Jao-ke Chin-Lee <jchinlee@chromium.org>
Date: Mon Aug 21 20:21:21 2017

Remove unneeded GetConfigSetURL.

GetConfigSetURL is only used for getting a display revision URL.
https://chromium-review.googlesource.com/c/608753 removes this use case (by
surfacing revision URL directly).

Bug:  747645 
Change-Id: Ie99ada1c6e97aa81b2f3ac34d0047ac3c93bc940
Reviewed-on: https://chromium-review.googlesource.com/608579
Commit-Queue: Jao-ke Chin-Lee <jchinlee@chromium.org>
Reviewed-by: Daniel Jacques <dnj@chromium.org>
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>

[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/appengine/backend/datastore/ds.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/appengine/backend/datastore/ds_test.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/appengine/gaeconfig/default_test.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/backend/backend.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/backend/caching/config.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/backend/caching/config_test.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/backend/client/client.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/backend/erroring/erroring.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/backend/testconfig/local_service_test.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/config.go
[modify] https://crrev.com/7a434b550599df6ea1425eb7789ebc6261d9056c/luci_config/server/cfgclient/config_test.go

Project Member

Comment 7 by bugdroid1@chromium.org, Aug 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-py.git/+/7bb180b5d2b1da6f0f688cf979b21bf9ff600c84

commit 7bb180b5d2b1da6f0f688cf979b21bf9ff600c84
Author: Jao-ke Chin-Lee <jchinlee@chromium.org>
Date: Thu Aug 24 18:32:00 2017

Store file url in File.

Previously, only the revision URL to the directory containing config files was
surfaced. This allows files to specify their own URL directly.

Bug:  747645 
Change-Id: Iab0fa7c6938e78541053e0d414b3c0efea060d1f
Reviewed-on: https://chromium-review.googlesource.com/633908
Commit-Queue: Jao-ke Chin-Lee <jchinlee@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/7bb180b5d2b1da6f0f688cf979b21bf9ff600c84/appengine/config_service/gitiles_import.py
[modify] https://crrev.com/7bb180b5d2b1da6f0f688cf979b21bf9ff600c84/appengine/config_service/gitiles_import_test.py
[modify] https://crrev.com/7bb180b5d2b1da6f0f688cf979b21bf9ff600c84/appengine/config_service/storage.py
[modify] https://crrev.com/7bb180b5d2b1da6f0f688cf979b21bf9ff600c84/appengine/config_service/storage_test.py

Status: Fixed (was: Assigned)
Waited for new Fuchsia jobs to show up (e.g. zircon-*) to confirm correct URL used.

Sign in to add a comment