New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 824558 link

Starred by 0 users
Status: Fixed
Owner:
qyears...@chromium.org
Closed: May 2018
Cc:
no...@chromium.org
vadimsh@chromium.org
phosek@chromium.org
juliehockett@google.com
qyears...@chromium.org
█ emso@chromium.org
mar...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 823940



Sign in to add a comment

Replace "ProjectDetails" in Tricium service config by adding fields to project config

Project Member Reported by qyears...@chromium.org, Mar 21 2018 
In general, project-specific config should be kept with project configs; there is no need to keep a list of projects in the service config.

Discussion in:
https://groups.google.com/a/google.com/forum/#!topic/luci-eng/zMZ3e8fnrUw

Concrete steps to take to make this change:
 - Add new fields to ProjectConfig, include support for multiple repos
 - Edit all existing project configs to include relevant info
 - Change Tricium Gerrit poller to fetch project configs from luci-config using the "get_project_configs" API
 - Change Tricium Gerrit poller to iterate through all Gerrit instances for each project (after this is done,  bug 823940  should be done, we will support multiple repos per project).
 - Make sure all handlers use the repo/gerrit details from the project configs, not from the service config
 - Remove all project details from service configs
 - Remove ProjectDetails from Tricium config proto

Comment 2 by qyears...@chromium.org, Mar 25 2018

Status: Started (was: Assigned)
Next step:
 - Edit all existing project configs to include relevant info

Comment 3 by qyears...@chromium.org, Mar 25 2018 

The specific project configs to edit:

 - playground & demo, devcfg
 - infra, luci-py, luci-go
 - gerrit
 - fuchsia-{garnet,peridot,topaz,zircon}

After editing these project repos, the next step is to use the API to get lists of project configs in tricium/appengine/gerrit/poll.go:
https://apis-explorer.appspot.com/apis-explorer/?base=https://luci-config.appspot.com/_ah/api#p/config/v1/config.get_project_configs
This should use the luci-config client library in the same way that Milo does:
https://chromium.googlesource.com/infra/luci/luci-go/+/fe4e304639d11ca00537768f8bfbf20ffecf73e6/milo/common/config.go#391
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 26 2018

Labels: merge-merged-config
The following revision refers to this bug:
  https://chromium.googlesource.com/playground/gerrit-tricium/demo/+/26df0ad0b08344e38ea29bf1caee794903af9532

commit 26df0ad0b08344e38ea29bf1caee794903af9532
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Mon Mar 26 20:43:44 2018

Expand demo project config for tricium to include repo details

Bug:  824558 
Change-Id: I7627f29ab14df670bd1a6e19782ac6ab1357d37a

[modify] https://crrev.com/26df0ad0b08344e38ea29bf1caee794903af9532/tricium-prod.cfg
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 26 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/playground/gerrit-tricium/+/9019c37998643d0d055bbf3042e0b153a6a40066

commit 9019c37998643d0d055bbf3042e0b153a6a40066
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Mon Mar 26 17:16:36 2018

Expand playground/gerrit-tricium project config to include repo details

Bug:  824558 
Change-Id: I13e8c21f1af194f5634340d04a96bb705bcabfa3

[modify] https://crrev.com/9019c37998643d0d055bbf3042e0b153a6a40066/tricium-dev.cfg
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 27 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/7545a23b7762b17652431489b10d6887d8220c30

commit 7545a23b7762b17652431489b10d6887d8220c30
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Tue Mar 27 20:18:44 2018

Expand infra project config for tricium to include new fields

All project details are being added into project configs for tricium;
https://chromium-review.googlesource.com/c/infra/infra/+/974543 added
some new fields, and this begins to add these fields for the infra.git
repo.

Bug:  824558 
Change-Id: I42c77a8bcb031924ca7ab5612b90e4a3c5dea8be
Reviewed-on: https://chromium-review.googlesource.com/979117
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/7545a23b7762b17652431489b10d6887d8220c30/tricium-dev.cfg
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 28 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/playground/gerrit-tricium/demo/+/48026f5f039e2b0ac1fea06e8de9fc9d2e6c25cb

commit 48026f5f039e2b0ac1fea06e8de9fc9d2e6c25cb
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Tue Mar 27 23:27:41 2018

Fix syntax error in demo repo tricium project config

Bug:  824558 
Change-Id: I55ad8b1af7494be9ddbdc7b9984f60da0ce6d217

[modify] https://crrev.com/48026f5f039e2b0ac1fea06e8de9fc9d2e6c25cb/tricium-prod.cfg

Comment 8 by qyears...@chromium.org, Apr 18 2018 

Planning to break this into at least 2 changes:

 - Add GetAllProjectConfigs and use it when polling gerrit
 - Change all instances of LookupProjectDetails
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 26 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/0bb48c3a835f36e570c62124d89603808378b356

commit 0bb48c3a835f36e570c62124d89603808378b356
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu Apr 26 23:45:25 2018

Add GetAllProjectConfigs and use it in gerrit poller

Bug:  824558 
Change-Id: I13af3b0671d3009b84a62a9c21c551903844c4d9
Reviewed-on: https://chromium-review.googlesource.com/1026885
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/gerrit/poll.go
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/gerrit/poll_test.go
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/common/track/db_test.go
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/launcher/rpc_launch_test.go
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/common/config/provider.go
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/devcfg/projects/playground-gerrit-tricium/tricium-dev.cfg
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/common/config/config.infra_testing
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/frontend/rpc_analyze_test.go
[modify] https://crrev.com/0bb48c3a835f36e570c62124d89603808378b356/go/src/infra/tricium/appengine/devcfg/projects/infra/tricium-dev.cfg
Project Member

Comment 10 by bugdroid1@chromium.org, May 1 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/f276a72c6753bb71c051b8f8e4e461da60e6cc7d

commit f276a72c6753bb71c051b8f8e4e461da60e6cc7d
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Tue May 01 15:41:33 2018

Change AnalyzeRequest to include GitRepo

Background:

We'd like to support multiple repositories per project config,
because this will generally be better for fuchsia (and possibly other
projects); it may make the way to configure things a little more
flexible, so you could have one config (which can include analyzer
definitions and selections) apply to a set of related configs
(I'm also thinking of infra repos here).

As part of  https://crbug.com/824558 , this change to the config
has now already been made.

But, if there can be more than one repo per project, then the
project name alone is not enough to get the repo details; we also
need to know which repo.  For example, when processing an analyze
request, we need to get the git repo so that we can make the initial
GIT_FILE_DETAILS data type.

All of this only applies to Git, since Git is the only supported
repo type currently.


This change proposes to make it so that AnalyzeRequest also
includes GitRepo, which should match the Repository field in
GitRepoDetails. The Gerrit poller would then use this repo URL
which making analyze requests. So, the poller will require both
git details and gerrit details in order to poll for a repository.

Bug:  824558 
Change-Id: I61271d918d7683ebadcf22a526c6b4430df59a86
Reviewed-on: https://chromium-review.googlesource.com/1033796
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/f276a72c6753bb71c051b8f8e4e461da60e6cc7d/go/src/infra/tricium/api/v1/tricium.proto
[modify] https://crrev.com/f276a72c6753bb71c051b8f8e4e461da60e6cc7d/go/src/infra/tricium/api/v1/pb.discovery.go
[modify] https://crrev.com/f276a72c6753bb71c051b8f8e4e461da60e6cc7d/go/src/infra/tricium/appengine/gerrit/poll_test.go
[modify] https://crrev.com/f276a72c6753bb71c051b8f8e4e461da60e6cc7d/go/src/infra/tricium/api/v1/tricium.pb.go
[modify] https://crrev.com/f276a72c6753bb71c051b8f8e4e461da60e6cc7d/go/src/infra/tricium/appengine/gerrit/poll.go
Project Member

Comment 11 by bugdroid1@chromium.org, May 7 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/9e4874d48a241080a5db1a54006bbfa06a32902a

commit 9e4874d48a241080a5db1a54006bbfa06a32902a
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Mon May 07 20:10:15 2018

[tricium] Add git/gerrit proto messages and LookupRepoDetails

This CL adds:
 - GerritRevision and GitCommit to AnalyzeRequest
 - GerritProject and GitRepo to RepoDetails in ProjectConfig
 - Helper method LookupRepoDetails, which relates these

This is one small part of overall changing the API:
Related doc:
https://docs.google.com/document/d/1nhLpTugZ5tfSJIZpEhwwh23UiSCK4NRdtEBV7Hhdffw/view

Bug:  824558 
Change-Id: I2ad8fbcd7b949d0939e104706b60c52b16f98930
Reviewed-on: https://chromium-review.googlesource.com/1043660
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/tricium.pb.go
[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/config.proto
[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/pb.discovery.go
[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/config.pb.go
[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/tricium.proto
[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/config_helpers.go
[modify] https://crrev.com/9e4874d48a241080a5db1a54006bbfa06a32902a/go/src/infra/tricium/api/v1/config_helpers_test.go
Project Member

Comment 13 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/276e87ae0a1286b1e01f1a875745a5918296af6e

commit 276e87ae0a1286b1e01f1a875745a5918296af6e
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 17:56:42 2018

Update track.AnalyzeRequest for AnalyzeRequest RPC change

This includes:

 - tricium.AnalyzeRequest has GitUrl, so we can change the name
   of GitRepo to GitURL to reflect this. URL is capitalized in
   the name as suggested in
   https://github.com/golang/go/wiki/CodeReviewComments#initialisms.
 - The GerritRevision field is redundant, replaced with GitRef.

Bug:  824558 
Change-Id: Ib62524b8b63318e5ec4281d4a00c17779be4a201
Reviewed-on: https://chromium-review.googlesource.com/1053795
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/common/track/track.go
[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/frontend/rpc_analyze.go
[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/driver/rpc_trigger.go
[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/driver/rpc_trigger_test.go
[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/frontend/run.go
[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/gerrit/rpc_report_results.go
[modify] https://crrev.com/276e87ae0a1286b1e01f1a875745a5918296af6e/go/src/infra/tricium/appengine/gerrit/rpc_report_results_test.go
Project Member

Comment 14 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/339f2822465af936bede18232199443d68750d28

commit 339f2822465af936bede18232199443d68750d28
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 21:19:43 2018

[tricium] Remove Name from ProjectConfig

When we have a project config, we either already have the name
because we requested it by name, or we already have the name
because we got all project configs together with their names.
Either way, we already have the name.

Bug:  824558 
Change-Id: I500af644bf05a1e8683f507063ecc0a6d22dbb31
Reviewed-on: https://chromium-review.googlesource.com/1054500
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/devcfg/projects/infra/tricium-dev.cfg
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/gerrit/poll_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/api/v1/tricium.pb.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/common/track/db_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/launcher/rpc_launch_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/api/v1/config.proto
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/api/v1/pb.discovery.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/api/v1/config.pb.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/common/config/validate.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/devcfg/projects/playground-gerrit-tricium/tricium-dev.cfg
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/frontend/rpc_analyze_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/common/config/validate_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/api/v1/config_helpers_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/common/config/generate_test.go
[modify] https://crrev.com/339f2822465af936bede18232199443d68750d28/go/src/infra/tricium/appengine/common/config/generate.go
Project Member

Comment 16 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/4015800b16b33e8890a1ff8dd322d1af1a8cc03d

commit 4015800b16b33e8890a1ff8dd322d1af1a8cc03d
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 22:59:03 2018

[tricium] Rework Gerrit/Git details in API and project config

In this change:
  - Remove all deprecated parts of schemas, including:
    - ProjectDetails
    - GitRepoDetails
    - GerritDetails
    - GerritConsumerDetails
  - Replace these with:
    - In project config: GerritProject/GitRepo
    - RPC requests: GerritRevision/GitCommit
  - Update validation
  - Update tests
  - Update example configs

This is a breaking change; when this is deployed,
we'll need to update configs and the plugin a the same time.

Bug:  824558 
Change-Id: Ic38e453b3bc279a5969562ce53149734b1851ce8
Reviewed-on: https://chromium-review.googlesource.com/1054175
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/common/track/track.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/admin/v1/pb.discovery.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/driver/rpc_trigger_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/pb.discovery.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/tricium.proto
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/common/config/generate_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/config_helpers_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/gerrit/poll_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/rpc_analyze.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/rpc_project_progress.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/rpc_progress_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/driver/rpc_trigger.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/devcfg/services/tricium-dev/service.cfg
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/config.proto
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/admin/v1/launcher.pb.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/devcfg/projects/playground-gerrit-tricium/tricium-dev.cfg
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/launcher/rpc_launch.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/devcfg/projects/infra/tricium-dev.cfg
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/config_helpers.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/gerrit/poll.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/tricium.pb.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/api/v1/config.pb.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/launcher/rpc_launch_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/run.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/common/config/validate_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/handlers_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/common/track/db_test.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/handlers.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/tracker/rpc_worker_done.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/rpc_progress.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/tracker/rpc_worker_launched.go
[modify] https://crrev.com/4015800b16b33e8890a1ff8dd322d1af1a8cc03d/go/src/infra/tricium/appengine/frontend/rpc_analyze_test.go
Project Member

Comment 17 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/playground/gerrit-tricium/+/30389340f384efcd1eef12f320eacb4b7af92280

commit 30389340f384efcd1eef12f320eacb4b7af92280
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 23:10:13 2018

[tricium] Update tricium config in playground repo

This changes gerrit_details -> gerrit_project and removes name.

Bug:  824558 
Change-Id: Ie105b778aca7d1a2702b29a961da0f949a489d76

[modify] https://crrev.com/30389340f384efcd1eef12f320eacb4b7af92280/tricium-dev.cfg
Project Member

Comment 18 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/playground/gerrit-tricium/demo/+/e189db0e778247b18cc9bf4a68985a8134c78dfb

commit e189db0e778247b18cc9bf4a68985a8134c78dfb
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 22:49:49 2018

[tricium] Update tricium config in demo repo

This changes gerrit_details -> gerrit_project and removes name.

Bug:  824558 
Change-Id: I9e7660996db90bf1a8d8a6dd2d448f014716feec

[modify] https://crrev.com/e189db0e778247b18cc9bf4a68985a8134c78dfb/tricium-prod.cfg
Project Member

Comment 19 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/2fe232376f7302ba85fb5b23ff40dabf010ff4af

commit 2fe232376f7302ba85fb5b23ff40dabf010ff4af
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 23:56:37 2018

[tricium] Update tricium config in infra repo

This changes gerrit_details -> gerrit_project and removes name.

Bug:  824558 
Change-Id: I06a20c7acf054dbfa1e61ae9bfe9e7caec7340ea
Reviewed-on: https://chromium-review.googlesource.com/1054137
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/2fe232376f7302ba85fb5b23ff40dabf010ff4af/tricium-dev.cfg
Project Member

Comment 20 by bugdroid1@chromium.org, May 10 2018 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infradata/config/+/79fd66d5f00b218cc7651d94a9401473e4895520

commit 79fd66d5f00b218cc7651d94a9401473e4895520
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Thu May 10 23:57:58 2018
Project Member

Comment 21 by bugdroid1@chromium.org, May 11 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/gerrit-plugins/tricium/+/c5d665afab38244f422b3ecc88f257ecd93050ac

commit c5d665afab38244f422b3ecc88f257ecd93050ac
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Fri May 11 14:31:34 2018

[tricium plugin] Update progress request for progress for new API

In order to streamline the AnalyzeRequest API and configs,
I'd like to replace the "gerrit_consumer_details" message
with a "gerrit_revision" message.

Server side CL: https://crrev.com/c/1054175.

As of that change, the service proto now contains something like:

    message ProgressRequest {
        oneof source {
            GerritRevision gerrit_revision
            // ... other possible options,
            // e.g. directly giving run ID
        }
    }

    message GerritRevision {
         string host
         string project
         string change
         string git_url // optional, not used in progress request
         string git_ref
    }

Bug:  824558 
Change-Id: I2c2bd2cd10ae6e48a8b8b4ad6d4e4cb8e070ff05
Reviewed-on: https://chromium-review.googlesource.com/1053367
Reviewed-by: Nodir Turakulov <nodir@chromium.org>

[modify] https://crrev.com/c5d665afab38244f422b3ecc88f257ecd93050ac/src/main/resources/static/tricium-client.js

Comment 22 by qyears...@chromium.org, May 11 2018 

Update: Now testing; this doesn't quite seem to be working yet, so I'll debug this morning.

Comment 23 by qyears...@chromium.org, May 11 2018 

Notes: 

The current part that I found that doesn't match expected behavior is that when repo details are parsed from the config, the result is not as expected.

For example, the infra repo config now contains:

repos {
  gerrit_project {
    host: "chromium-review.googlesource.com"
    project: "infra/infra"
    git_url: "https://chromium.googlesource.com/infra/infra"
  }
}

But the parsed repo details look like this:

&tricium.RepoDetails{
  Source:tricium.isRepoDetails_Source(nil),
  DisableReporting:false,
  WhitelistedGroup:[]string{
    "\n chromium-review.googlesource.com\x12\vinfra/infra\x1a-https://chromium.googlesource.com/infra/infra"
  }
}

(No source, and all of the actual content is put into the whitelisted_group field.)

Comment 24 by qyears...@chromium.org, May 11 2018

Cc: vadimsh@chromium.org
When I run this with a local devserver, which uses the locally-checked in configs, which are similar to the above, e.g.

repos {
  gerrit_project {
    host: "chromium-review.googlesource.com"
    project: "playground/gerrit-tricium"
    git_url: "https://chromium.googlesource.com/playground/gerrit-tricium"
  }
  disable_reporting: true
}

And then try to poll, it parses as expected:

&tricium.RepoDetails{
  Source:(*tricium.RepoDetails_GerritProject)(0xc42057c1b8), 
  DisableReporting:true,
  WhitelistedGroup:[]string(nil)
}

So I believe that the format of the config is correct, and the problem has something to do with parsing the config fetched via luci-config.

+Vadim
General question: does anyone know if this has happened before, where some config schema is updated and then when making requests to luci-config (in this case by a call to cfgclient.Projects using go.chromium.org/luci/config/server/cfgclient) the config is not parsed as expected?
Project Member

Comment 25 by bugdroid1@chromium.org, May 11 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/28952037923e9481c759c9a4e708f71ab0662fb4

commit 28952037923e9481c759c9a4e708f71ab0662fb4
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Fri May 11 21:47:19 2018

[tricium] Validate fields of GitCommit and add tests

This is a follow-up to the main CL which introduced
GitCommit/GerritRevision.

Bug:  824558 
Change-Id: Ie98b58d14fac0105cad991f7bcdfe86aa667c007
Reviewed-on: https://chromium-review.googlesource.com/1054784
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/28952037923e9481c759c9a4e708f71ab0662fb4/go/src/infra/tricium/appengine/frontend/rpc_analyze_test.go
[modify] https://crrev.com/28952037923e9481c759c9a4e708f71ab0662fb4/go/src/infra/tricium/appengine/frontend/rpc_analyze.go
[modify] https://crrev.com/28952037923e9481c759c9a4e708f71ab0662fb4/go/src/infra/tricium/appengine/frontend/frontend.infra_testing

Comment 26 by vadimsh@chromium.org, May 11 2018 

You shouldn't have reused field tags in https://chromium-review.googlesource.com/c/infra/infra/+/1054175/9/go/src/infra/tricium/api/v1/config.proto. It broke the binary backward compatibility of messages. In particular, binary protos stored in the luci-config library cache are not gibberish. Presumably should should be evicted from the cache eventually, but I don't know how exactly this works.

Comment 27 by vadimsh@chromium.org, May 11 2018 

*are now gibberish

Comment 28 by qyears...@chromium.org, May 11 2018 

Ah, that makes sense! -- Nodir actually asked me in code review whether binary protos were used anywhere, and I thought probably not, but I was wrong :-P

So, would you recommend changing the proto definition again so that there are no re-used field tags, and the new fields get new numbers? Or perhaps is there some to clear the cache that might be easier?

Comment 29 by vadimsh@chromium.org, May 11 2018 

Theoretically updating configs should be sufficient to eventually replace all caches. Does infra/infra config still appear broken?

Comment 30 by qyears...@chromium.org, May 11 2018 

To check my understanding: if updating configs is sufficient to replace all caches, then if the service is updated and then the configs are updated, then the configs should be OK, right? The way to get into a broken state would be to update configs and then update service afterwards? and the way to fix it then would be re-import configs in luci-config?

Comment 31 by vadimsh@chromium.org, May 11 2018 

> then if the service is updated and then the configs are updated, then the configs should be OK, right?

Correct. In between these two events the service may see old cached configs which may appear broken.

> The way to get into a broken state would be to update configs and then update service afterwards?

This should not be possible if you have config validation enabled, since old service will reject new configs. If the validation is not enabled, the following is potentially possible (depending on how configs changed):

1. Old service sees new configs, stores them in the cache using old proto schema.
2. Old service is updated to new service.
3. New service sees that config are already imported, and even cached. So it doesn't do anything.
4. But it doesn't realize the cached configs are in OLD proto schema, and they deserialize as gibberish in NEW schema.

If this is what indeed happened, then you'll need to make a whitespace change to all configs to make sure new service reimports them and stores them with new schema.
Project Member

Comment 32 by bugdroid1@chromium.org, May 12 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/infra/+/1edaf3d04874bb677da15f1f02ac1dc3807e2dde

commit 1edaf3d04874bb677da15f1f02ac1dc3807e2dde
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Sat May 12 01:21:09 2018

[tricium] Change config proto field numbers to avoid re-use

Vadim noted in the bug that binary protos for configs
are actually stored/cached, which means that changing
the numbers associated with fields will break things
(at least temporarily).

This changes the config back so that it uses the same
numbers for the same fields as before and new numbers
for new fields. This CL also removes a comment that
should have been removed before.

Bug:  824558 
Change-Id: I458c2c2a7257ad196ed32925e15b9da75a769959
Reviewed-on: https://chromium-review.googlesource.com/1056206
Reviewed-by: Vadim Shtayura <vadimsh@chromium.org>
Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>

[modify] https://crrev.com/1edaf3d04874bb677da15f1f02ac1dc3807e2dde/go/src/infra/tricium/api/v1/pb.discovery.go
[modify] https://crrev.com/1edaf3d04874bb677da15f1f02ac1dc3807e2dde/go/src/infra/tricium/api/v1/config.proto
[modify] https://crrev.com/1edaf3d04874bb677da15f1f02ac1dc3807e2dde/go/src/infra/tricium/api/v1/config.pb.go
Project Member

Comment 33 by bugdroid1@chromium.org, May 12 2018 

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/infradata/config/+/846f47d869c799e5896b01dd9e02c64534fc7c5e

commit 846f47d869c799e5896b01dd9e02c64534fc7c5e
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Sat May 12 15:52:25 2018

Comment 34 by qyears...@chromium.org, May 12 2018

Status: Fixed (was: Started)

Comment 35 by qyears...@chromium.org, May 15 2018

Blocking: 823940
►

Sign in to add a comment