|
|
Created:
3 years, 4 months ago by iannucci Modified:
3 years, 4 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
DescriptionAdd source manifest proto to recipe_engine.
This adds the initial form of the "source manifest" proto which will
allow recipe_engine to communicate structured data about
stuff-on-disk-that-we-got-from-remote-servers to LUCI services like Milo,
and will also give us a better option than 'got_revision' properties for
exporting this data from recipes (i.e. we'll be able to export all the
data and then query/read it instead of hard-coding a few hacks to export
specific repos).
This only covers git, cipd and isolated for now, but could be expanded
to other data sources, if needed. We don't currently anticipate a need
for that, but the proto is hopefully obviously extensible for this.
Clients consuming this proto (like Milo) may have specialized support
when comparing two manifest (e.g. two entries sharing the same git repo
may render as a git log), but it should also be somewhat easy to show a
useful generic diff when comparing two Manifest protos, even when the
deployment type changes (i.e. local path "foo" used to be a git checkout,
but is now multiple CIPD packages).
R=dnj@chromium.org, hinoka@chromium.org, machenbach@chromium.org
BUG=468053
Review-Url: https://codereview.chromium.org/2998523002
Committed: https://github.com/luci/recipes-py/commit/e92def4f3c06e6dc65c8d69f6bbb378ccba92546
Patch Set 1 #Patch Set 2 : source manifest #Patch Set 3 : Rename some stuff #
Total comments: 2
Patch Set 4 : fix stuff #
Total comments: 5
Patch Set 5 : fix nits #Patch Set 6 : typo #
Total comments: 6
Patch Set 7 : directories #Patch Set 8 : comment on fetch_url #Messages
Total messages: 20 (7 generated)
Description was changed from ========== Add deployment manifest proto to recipe_engine. This adds the initial form of the "deployment manifest" proto which will allow recipe_engine to communicate structured data about stuff-on-disk-that-we-got-from-remote-servers to LUCI services like Milo, and will also give us a better option than 'got_revision' properties for exporting this data from recipes (i.e. we'll be able to export all the data and then query/read it instead of hard-coding a few hacks to export specific repos). This only covers git, cipd and isolated for now, but could be expanded to other data sources, if needed. We don't currently anticipate a need for that, but the proto is hopefully obviously extensible for this. Clients consuming this proto (like Milo) may have specialized support when comparing two manifest (e.g. two entries sharing the same git repo may render as a git log), but it should also be somewhat easy to show a useful generic diff when comparing two Manifest protos, even when the deployment type changes (i.e. local path "foo" used to be a git checkout, but is now multiple CIPD packages). R=dnj@chromium.org, hinoka@chromium.org, machenbach@chromium.org BUG=468053 ========== to ========== Add source manifest proto to recipe_engine. This adds the initial form of the "source manifest" proto which will allow recipe_engine to communicate structured data about stuff-on-disk-that-we-got-from-remote-servers to LUCI services like Milo, and will also give us a better option than 'got_revision' properties for exporting this data from recipes (i.e. we'll be able to export all the data and then query/read it instead of hard-coding a few hacks to export specific repos). This only covers git, cipd and isolated for now, but could be expanded to other data sources, if needed. We don't currently anticipate a need for that, but the proto is hopefully obviously extensible for this. Clients consuming this proto (like Milo) may have specialized support when comparing two manifest (e.g. two entries sharing the same git repo may render as a git log), but it should also be somewhat easy to show a useful generic diff when comparing two Manifest protos, even when the deployment type changes (i.e. local path "foo" used to be a git checkout, but is now multiple CIPD packages). R=dnj@chromium.org, hinoka@chromium.org, machenbach@chromium.org BUG=468053 ==========
vadimsh@chromium.org changed reviewers: + vadimsh@chromium.org
https://codereview.chromium.org/2998523002/diff/40001/recipe_engine/source_ma... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/40001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:98: bytes hash = 2; 3
LGTM with nits on the git and isolate stuff. Don't know much about CIPD. Q: Is this mainly for infra or source or both? Q: If also for source, note that during the run of one recipe, the manifest of source might change several times. Should it be represented then by many different manifest instances? E.g. right after bot_update, we know the source revisions, but not the isolate hashes yet. The with/without patch bot_update revisions differ. A recipe that does bisection, might checkout many revisions throughout the execution. Note also that e.g. bot_update has instructions on when to change UI properties on changed checkout data and when not. Bot_update with patch updates the presentation (i.e. sets got_*_revision properties), but bot_update without patch doesn't. This is probably orthogonal to the representation though... https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:27: // version 0 is the only version. Version will increment on backwards-incompatible nit: I don't understand what's meant with "version 0 is the only version". https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:53: // This should always be the refo on the hosted repo (not any local alias s/refo/ref
On 2017/08/07 07:01:38, Michael Achenbach wrote: > LGTM with nits on the git and isolate stuff. Don't know much about CIPD. > > Q: Is this mainly for infra or source or both? Both. Specifically, infra will upload 1-2 manifests (manifest names TBD, but probably something like "infra.buildbucket_job" and "recipes.source"), and then the recipe itself (i.e. bot_update, git.checkout, etc.) can upload additional manifests as well (e.g. "unpatched", "patched", "bisect:<identifier>", etc.). > > Q: If also for source, note that during the run of one recipe, the manifest of > source might change several times. Should it be represented then by many > different manifest instances? E.g. right after bot_update, we know the source > revisions, but not the isolate hashes yet. The with/without patch bot_update > revisions differ. A recipe that does bisection, might checkout many revisions > throughout the execution. > Yes; each manifest has a unique name and is a full snapshot of all the sources for that 'checkout' at that point in time. The protocol allows for arbitrary name strings, but we'll want to define some convention for them so they don't stomp on each other (exact conventions TBD). > Note also that e.g. bot_update has instructions on when to change UI properties > on changed checkout data and when not. Bot_update with patch updates the > presentation (i.e. sets got_*_revision properties), but bot_update without patch > doesn't. This is probably orthogonal to the representation though... I'd like to make bot_update always upload /some/ manifest; the got_properties will continue to exist until people stop needing/using them. If we always upload manifests for each source sync action, then we won't need to invent new weird properties any time we realize we want to export some additional revision. > > https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... > File recipe_engine/source_manifest.proto (right): > > https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... > recipe_engine/source_manifest.proto:27: // version 0 is the only version. > Version will increment on backwards-incompatible > nit: I don't understand what's meant with "version 0 is the only version". > > https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... > recipe_engine/source_manifest.proto:53: // This should always be the refo on the > hosted repo (not any local alias > s/refo/ref
PTAL https://codereview.chromium.org/2998523002/diff/40001/recipe_engine/source_ma... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/40001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:98: bytes hash = 2; On 2017/08/05 01:29:13, Vadim Sh. wrote: > 3 I definitely compiled the proto.... ._. https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:27: // version 0 is the only version. Version will increment on backwards-incompatible On 2017/08/07 07:01:37, Michael Achenbach wrote: > nit: I don't understand what's meant with "version 0 is the only version". Clearer? https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:53: // This should always be the refo on the hosted repo (not any local alias On 2017/08/07 07:01:37, Michael Achenbach wrote: > s/refo/ref Done.
FYI, still looking for a review from dnj, hinoka and/or vadimsh :)
https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/60001/recipe_engine/source_ma... recipe_engine/source_manifest.proto:27: // version 0 is the only version. Version will increment on backwards-incompatible On 2017/08/07 19:02:28, iannucci wrote: > On 2017/08/07 07:01:37, Michael Achenbach wrote: > > nit: I don't understand what's meant with "version 0 is the only version". > > Clearer? Jep
lgtm with a bunch of questions: 1. Is 'cipd_package_pattern' actually easy to get? 2. How 'deployments' are sorted? Do we want some stable sorting order there?
hinoka@google.com changed reviewers: + hinoka@google.com
lgtm https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... recipe_engine/source_manifest.proto:1: // Copyright 2016 The LUCI Authors. All rights reserved. nit: ring in the new year https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... recipe_engine/source_manifest.proto:42: // https://chromium.googlesource.com/external/github.com/luci/recipes-py If this is the same as repo_url, should it be empty or a copy? https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... recipe_engine/source_manifest.proto:49: bytes revision = 3; in theory bytes makes sense, only time will tell if this will trip us up...
On 2017/08/08 19:19:14, Vadim Sh. wrote: > lgtm with a bunch of questions: > > 1. Is 'cipd_package_pattern' actually easy to get? Not currently, but stuff like run_isolated potentially knows; As I build users of this proto, I think this information will be available in more places than not. > 2. How 'deployments' are sorted? Do we want some stable sorting order there? It's up to the implementation, e.g. bot_update, to always produce its manifest in the same order, but I can't think of a sensible sorting scheme that would make sense across clients (i.e. bot_update and git.checkout, or something). I think for protocol purposes this is fine, and if we identify things where having a common sort order would make sense we can just tweak the clients and add a comment to the proto.
https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... File recipe_engine/source_manifest.proto (right): https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... recipe_engine/source_manifest.proto:1: // Copyright 2016 The LUCI Authors. All rights reserved. On 2017/08/08 19:55:28, Ryan Tseng wrote: > nit: ring in the new year Done. https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... recipe_engine/source_manifest.proto:42: // https://chromium.googlesource.com/external/github.com/luci/recipes-py On 2017/08/08 19:55:28, Ryan Tseng wrote: > If this is the same as repo_url, should it be empty or a copy? Done. https://codereview.chromium.org/2998523002/diff/100001/recipe_engine/source_m... recipe_engine/source_manifest.proto:49: bytes revision = 3; On 2017/08/08 19:55:28, Ryan Tseng wrote: > in theory bytes makes sense, only time will tell if this will trip us up... Acknowledged.
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org, vadimsh@chromium.org, hinoka@google.com Link to the patchset: https://codereview.chromium.org/2998523002/#ps140001 (title: "comment on fetch_url")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1502228612180420, "parent_rev": "e0ddd60a4c230f913a8032eadd3c5a46cb944c42", "commit_rev": "e92def4f3c06e6dc65c8d69f6bbb378ccba92546"}
Message was sent while issue was closed.
Description was changed from ========== Add source manifest proto to recipe_engine. This adds the initial form of the "source manifest" proto which will allow recipe_engine to communicate structured data about stuff-on-disk-that-we-got-from-remote-servers to LUCI services like Milo, and will also give us a better option than 'got_revision' properties for exporting this data from recipes (i.e. we'll be able to export all the data and then query/read it instead of hard-coding a few hacks to export specific repos). This only covers git, cipd and isolated for now, but could be expanded to other data sources, if needed. We don't currently anticipate a need for that, but the proto is hopefully obviously extensible for this. Clients consuming this proto (like Milo) may have specialized support when comparing two manifest (e.g. two entries sharing the same git repo may render as a git log), but it should also be somewhat easy to show a useful generic diff when comparing two Manifest protos, even when the deployment type changes (i.e. local path "foo" used to be a git checkout, but is now multiple CIPD packages). R=dnj@chromium.org, hinoka@chromium.org, machenbach@chromium.org BUG=468053 ========== to ========== Add source manifest proto to recipe_engine. This adds the initial form of the "source manifest" proto which will allow recipe_engine to communicate structured data about stuff-on-disk-that-we-got-from-remote-servers to LUCI services like Milo, and will also give us a better option than 'got_revision' properties for exporting this data from recipes (i.e. we'll be able to export all the data and then query/read it instead of hard-coding a few hacks to export specific repos). This only covers git, cipd and isolated for now, but could be expanded to other data sources, if needed. We don't currently anticipate a need for that, but the proto is hopefully obviously extensible for this. Clients consuming this proto (like Milo) may have specialized support when comparing two manifest (e.g. two entries sharing the same git repo may render as a git log), but it should also be somewhat easy to show a useful generic diff when comparing two Manifest protos, even when the deployment type changes (i.e. local path "foo" used to be a git checkout, but is now multiple CIPD packages). R=dnj@chromium.org, hinoka@chromium.org, machenbach@chromium.org BUG=468053 Review-Url: https://codereview.chromium.org/2998523002 Committed: https://github.com/luci/recipes-py/commit/e92def4f3c06e6dc65c8d69f6bbb378ccba... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://github.com/luci/recipes-py/commit/e92def4f3c06e6dc65c8d69f6bbb378ccba... |