New issue
Advanced search Search tips

Issue 811763 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

improve api for setting properties

Project Member Reported by no...@chromium.org, Feb 13 2018

Issue description

Using current property API, it is impossible to set property ["a", "b"] in one step and ["a", "c"] in another, i.e. 

{
  "a": {"b": 1, "c": 1}
}

this is getting more important if we want users to use property as output data, as opposed to, e.g., step links. Specifically, it is impossible to generate

{
  "steps": {
    "browser_tests": {
       "tasks": [{...},...],
    },
    "other steps": {...}
  }
}
 

Comment 1 by no...@chromium.org, Feb 13 2018

Description: Show this description

Comment 2 by no...@chromium.org, Feb 13 2018

more examples, ShamWow (new test result storage) is being designed now. In
https://docs.google.com/a/google.com/document/d/1bWtKtR4hJzWMIvXu6lyOjerukxpTcnOKjU2d-D1rOKM/edit?disco=AAAABenVsnQ
I've suggested to listen to completed builds and read test task ids from properties. So they'd need the structure like above. Otherwise they will have to do something uglier, like {
  "steps.browser_tests": {},
  "steps.other step": {},
}

Comment 3 by no...@chromium.org, Feb 16 2018

interesting use case: if a recipe module sets a property, and a recipe needs to use it multiple times, each invocation overwrites (or spoils) previous property values. We might want to add some sort of property path prefixing, e.g. 

  with api.context(property_prefix=['a', 'b0']): 
    # set property c=1
  with api.context(property_prefix=['a', 'b1']): 
    # set property c=2

result: {
  'a': {
    'b0': {'c': 1},
    'b1': {'c': 2},
  },
}
So I think the simplest implementation would look something like:

  api.state #<- is a dict() and users can tweak it however they like

However, I truly worry about `api.state` turning into a kind of global variable passing scheme (i.e. with module `A` passing data to module `B` through this). This is exactly what `properties` used to be prior to recipes. It was pretty bad.

We could prevent this by giving each recipe module a `state` dictionary, and also putting a `state` dictionary directly on the api object that RunSteps get, and then having the recipe engine compose the results into a single dictionary.

Then the output properties would look like:

  {
    "property": ["from", "RunSteps"],
    "$build/swarming": {
       "some": "thing",
       "from": "the swarming module",
    },
  }

This would prevent accidental (or intentional) global variable passing; everything would be firewalled to its own domain by default. You never have to globally coordinate namespaces. It does make all the data segmented by module instead of step though... this division may not make sense. To mitigate that, users might pass sub-keys of their own state for subroutines to write into (e.g. the recipe might pass `api.state['task_ids']` down the stack for things to append to). I can't tell if that's messy or elegant. A better form might be to pass e.g. the swarming module a callback function which closes over the higher level state object. Swarming module calls you back with important stuff (e.g. tasks it triggered) and your callback function records the relevant bits of info.

If I had my druthers, I'd force every recipe to pre-declare its output data as a proto. If we did this, then I would feel much better at just letting this object hang around for open access. Then it would be hard to do something really embarrassing (since you'd have to get someone to +1 your global variable in the proto file, and your shame would be open for all to see :p).

Comment 5 by st...@chromium.org, Feb 16 2018

Re the case in #3:
In CQ, first pass is checkout headA, build and run test "with patch", and properties or the checkout manifests are bound to tests at headA. If tests failed with patch, the recipe will checkout headB (newer than headA), build and run test "without patch", and properties or manifests of the same names are set again.

If we care about the difference of properties values here in some further processing, it might worth to prevent the properties or manifests being overwritten.

Comment 6 by estaab@chromium.org, Feb 22 2018

Status: Available (was: Untriaged)
Cc: -iannucci@chromium.org iannu...@google.com

Sign in to add a comment