Standardize proto package names of LUCI services |
||
Issue descriptionThis is important if we want to use protos relating to multiple different services in one binary/script. For example, trying to use both luci-milo and luci-notify protos from one binary doesn't work currently, because both use proto package named "config", and both define message named "ProjectConfig", so we end up with two config.ProjectConfig messages in one binary. At least go protobuf lib doesn't allow this. Proposal: 1. Use luci.<service>.config as proto package name for all proto messages that end up in LUCI configuration files (project level or service level). 2. Use luci.<service>.internal for all internal protos (the ones that do not leave boundaries of the service). 3. Use luci.<service>.api for protos with API service definitions. The last item unfortunately introduces incompatible change in pRPC APIs since they use proto package name as part of the URL :( I'm going to do at least (1) for now, since this is needed for Skylark config generator.
,
May 14 2018
Few more potential gotchas: 1. Go's luci-config thick client uses fully qualified proto message names as part of datastore cache key. Changing proto package invalidates caches. I believe by default all luci-go services use "fail open" strategy [1] for cache missies, so everything should just work. Logdog though seems to be using "fail closed", so I can't predict what will happen if we rename logdog config protos. 2. 'tq' library uses fully qualified proto message names as part of task queue tasks. So its currently non-trivial to rename proto messages used to represent tq tasks.
,
May 15 2018
After discussion with nodir@ we decided to do less renames. Now the only requirement is that protos for service X leave in proto package X or somewhere under package X. In particular all "public" messages (API definitions, config protos) should live directly in package X. This makes all our pRPC definitions already compliant with the new rule.
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-go.git/+/11b743c7a69749e450100814670eab2fb73d6df3 commit 11b743c7a69749e450100814670eab2fb73d6df3 Author: Vadim Shtayura <vadimsh@chromium.org> Date: Tue May 15 01:46:38 2018 [scheduler] Rename proto package with configs to 'scheduler'. See the bug for the rationale. R=tandrii@chromium.org, nodir@chromium.org BUG= 842920 Change-Id: I4da8c606a82ed560fd3f1b49d1eaaf3129f0d373 Reviewed-on: https://chromium-review.googlesource.com/1058559 Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> Commit-Queue: Vadim Shtayura <vadimsh@chromium.org> [modify] https://crrev.com/11b743c7a69749e450100814670eab2fb73d6df3/scheduler/appengine/messages/config.pb.go [modify] https://crrev.com/11b743c7a69749e450100814670eab2fb73d6df3/scheduler/appengine/messages/config.proto [modify] https://crrev.com/11b743c7a69749e450100814670eab2fb73d6df3/scheduler/appengine/messages/cron.pb.go [modify] https://crrev.com/11b743c7a69749e450100814670eab2fb73d6df3/scheduler/appengine/messages/cron.proto
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-py.git/+/7fb14584fc00e85e3414302b64d8f6cfbb6747a8 commit 7fb14584fc00e85e3414302b64d8f6cfbb6747a8 Author: Vadim Shtayura <vadimsh@chromium.org> Date: Tue May 15 20:43:35 2018 [luci-config] Convert *.proto to proto3 and rename the proto package. For config files we follow package name convention "<service>". So for luci-config service, the package name ends up being just "config". R=nodir@chromium.org, maruel@chromium.org BUG= 842920 Change-Id: Ib4d8e49dc63dcb9402c5be3a006c5b4c77d989e7 Reviewed-on: https://chromium-review.googlesource.com/1058669 Reviewed-by: Marc-Antoine Ruel <maruel@chromium.org> Commit-Queue: Vadim Shtayura <vadimsh@chromium.org> [modify] https://crrev.com/7fb14584fc00e85e3414302b64d8f6cfbb6747a8/appengine/components/components/config/proto/Makefile [modify] https://crrev.com/7fb14584fc00e85e3414302b64d8f6cfbb6747a8/appengine/components/components/config/proto/project_config.proto [modify] https://crrev.com/7fb14584fc00e85e3414302b64d8f6cfbb6747a8/appengine/components/components/config/proto/project_config_pb2.py [modify] https://crrev.com/7fb14584fc00e85e3414302b64d8f6cfbb6747a8/appengine/components/components/config/proto/service_config.proto [modify] https://crrev.com/7fb14584fc00e85e3414302b64d8f6cfbb6747a8/appengine/components/components/config/proto/service_config_pb2.py
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-go.git/+/9e27f8c5005b7d5fbbbf4e158d30dc02557be9cc commit 9e27f8c5005b7d5fbbbf4e158d30dc02557be9cc Author: Vadim Shtayura <vadimsh@chromium.org> Date: Tue May 15 22:21:29 2018 [milo] Rename proto package with configs to 'milo'. See the bug for the rationale. R=hinoka@chromium.org, maruel@chromium.org BUG= 842920 Change-Id: Ie11d4fecaf40d41d863ef9863b3bdedba6a8cf75 Reviewed-on: https://chromium-review.googlesource.com/1060473 Reviewed-by: Ryan Tseng <hinoka@chromium.org> Commit-Queue: Vadim Shtayura <vadimsh@chromium.org> [modify] https://crrev.com/9e27f8c5005b7d5fbbbf4e158d30dc02557be9cc/milo/api/config/project.pb.go [modify] https://crrev.com/9e27f8c5005b7d5fbbbf4e158d30dc02557be9cc/milo/api/config/project.proto [modify] https://crrev.com/9e27f8c5005b7d5fbbbf4e158d30dc02557be9cc/milo/api/config/settings.pb.go [modify] https://crrev.com/9e27f8c5005b7d5fbbbf4e158d30dc02557be9cc/milo/api/config/settings.proto
,
May 15 2018
The following revision refers to this bug: https://chromium.googlesource.com/infra/luci/luci-go.git/+/fb2f9f171639eccca12ea3805463eeed6e74c214 commit fb2f9f171639eccca12ea3805463eeed6e74c214 Author: Vadim Shtayura <vadimsh@chromium.org> Date: Tue May 15 22:24:39 2018 [notify] Rename proto package with configs to 'notify'. See the bug for the rationale. R=maruel@chromium.org, nodir@chromium.org BUG= 842920 Change-Id: I7acf12d1afd7306bdbc20a38298f963a58e17e55 Reviewed-on: https://chromium-review.googlesource.com/1060474 Reviewed-by: Nodir Turakulov <nodir@chromium.org> Commit-Queue: Vadim Shtayura <vadimsh@chromium.org> [modify] https://crrev.com/fb2f9f171639eccca12ea3805463eeed6e74c214/luci_notify/api/config/notify.pb.go [modify] https://crrev.com/fb2f9f171639eccca12ea3805463eeed6e74c214/luci_notify/api/config/notify.proto [modify] https://crrev.com/fb2f9f171639eccca12ea3805463eeed6e74c214/luci_notify/api/config/settings.pb.go [modify] https://crrev.com/fb2f9f171639eccca12ea3805463eeed6e74c214/luci_notify/api/config/settings.proto
,
May 22 2018
Good enough for now. |
||
►
Sign in to add a comment |
||
Comment 1 by vadimsh@chromium.org
, May 14 2018