Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668113002:
step: expose get_context() to get the context object (Closed)
|
Created:
3 years, 10 months ago by Paweł Hajdan Jr. Modified:
3 years, 10 months ago CC:
chromium-reviews, infra-reviews+recipes-py_chromium.org Target Ref:
refs/heads/master Project:
recipe_engine Visibility:
Public. |
Descriptionstep: expose methods to read values from context
See https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/w__b1zTWAQAJ
for more context, specifically https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/efjINUEnBgAJ .
BUG=685746
Review-Url: https://codereview.chromium.org/2668113002
Committed: https://github.com/luci/recipes-py/commit/0e4d4e97edb04696b0b1452522eb26f1dc633e8e
Patch Set 1 #
Total comments: 2
Patch Set 2 : fixes #
Total comments: 6
Patch Set 3 : fixes #Patch Set 4 : docs #
Total comments: 5
Patch Set 5 : comments #Messages
Total messages: 36 (26 generated)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
phajdan.jr@chromium.org changed reviewers: + iannucci@chromium.org, martiniss@chromium.org
https://codereview.chromium.org/2668113002/diff/1/recipe_modules/step/api.py File recipe_modules/step/api.py (right): https://codereview.chromium.org/2668113002/diff/1/recipe_modules/step/api.py#... recipe_modules/step/api.py:81: return recipe_api._STEP_CONTEXT this should return a copy of the step context (unless _STEP_CONTEXT is immutable? I forget). needs docstring
https://codereview.chromium.org/2668113002/diff/1/recipe_modules/step/api.py File recipe_modules/step/api.py (right): https://codereview.chromium.org/2668113002/diff/1/recipe_modules/step/api.py#... recipe_modules/step/api.py:81: return recipe_api._STEP_CONTEXT This is also confusing with the previous property. Maybe current_context?
Description was changed from ========== step: expose get_context() to get the context object See https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/w__b1zTW... for more context, specifically https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/efjINUEn... . This CL is deliberately small and simple so that downstream can start using it before we make any breaking changes. BUG=685746 ========== to ========== step: expose methods to read values from context See https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/w__b1zTW... for more context, specifically https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/efjINUEn... . BUG=685746 ==========
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL, updated patch based on suggestions and IM discussion.
LGTM, I feel a lot better about not returning the whole context object back to the user and just pulling values from it. https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api.py File recipe_modules/step/api.py (right): https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api... recipe_modules/step/api.py:87: """Returns |key| from context if present, otherwise |default|.""" Returns |key|'s value from context... https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api... recipe_modules/step/api.py:91: """Combines |value| with the value for |key| in current context, if any.""" ... and returns the combined value. It would be really good to include all the relevant keys and their 'combine' functions in the docstring for the context property above. Probably just env and cwd? IIRC, cwd's combine is just replacement, but env does a dict.update? https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api... recipe_modules/step/api.py:195: full_name = self.combine_with_context('name', name) ew, I forgot about name. this is used to implement name prefixing, right?
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api.py File recipe_modules/step/api.py (right): https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api... recipe_modules/step/api.py:87: """Returns |key| from context if present, otherwise |default|.""" On 2017/02/01 17:47:00, iannucci wrote: > Returns |key|'s value from context... Done. https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api... recipe_modules/step/api.py:91: """Combines |value| with the value for |key| in current context, if any.""" On 2017/02/01 17:47:00, iannucci wrote: > ... and returns the combined value. Done. > It would be really good to include all the relevant keys and their 'combine' > functions in the docstring for the context property above. Probably just env and > cwd? I'm worried about the docstring getting out of sync with reality. There are more possible keys than just env and cwd. > IIRC, cwd's combine is just replacement, but env does a dict.update? Yup. https://codereview.chromium.org/2668113002/diff/20001/recipe_modules/step/api... recipe_modules/step/api.py:195: full_name = self.combine_with_context('name', name) On 2017/02/01 17:47:00, iannucci wrote: > ew, I forgot about name. this is used to implement name prefixing, right? Yes, see api.step.nest .
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Recipes-py Presubmit on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/34139f0983952c10)
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2668113002/diff/60001/recipe_modules/step/api.py File recipe_modules/step/api.py (right): https://codereview.chromium.org/2668113002/diff/60001/recipe_modules/step/api... recipe_modules/step/api.py:85: cwd - working directory (Path object from api.path) https://codereview.chromium.org/2668113002/diff/60001/recipe_modules/step/api... recipe_modules/step/api.py:86: env - environment variables ({var -> pattern}) https://codereview.chromium.org/2668113002/diff/60001/recipe_modules/step/api... recipe_modules/step/api.py:87: infra_step - whether the step failure should be marked as infra failure (bool) https://codereview.chromium.org/2668113002/diff/60001/recipe_modules/step/api... recipe_modules/step/api.py:88: name - step name (str) Isn't this the step name prefix? https://codereview.chromium.org/2668113002/diff/60001/recipe_modules/step/api... recipe_modules/step/api.py:89: nest_level - level of step nesting (see api.step.nest) I would remove this one; users should use api.step.nest instead, I think?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by phajdan.jr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by phajdan.jr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from iannucci@chromium.org Link to the patchset: https://codereview.chromium.org/2668113002/#ps80001 (title: "comments")
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": 80001, "attempt_start_ts": 1485981636445270,
"parent_rev": "4fb4d4bdfda85253b842648abf8588749c68a5eb", "commit_rev":
"0e4d4e97edb04696b0b1452522eb26f1dc633e8e"}
Message was sent while issue was closed.
Description was changed from ========== step: expose methods to read values from context See https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/w__b1zTW... for more context, specifically https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/efjINUEn... . BUG=685746 ========== to ========== step: expose methods to read values from context See https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/w__b1zTW... for more context, specifically https://groups.google.com/a/chromium.org/d/msg/infra-dev/p8Iq9v9Y4k0/efjINUEn... . BUG=685746 Review-Url: https://codereview.chromium.org/2668113002 Committed: https://github.com/luci/recipes-py/commit/0e4d4e97edb04696b0b1452522eb26f1dc6... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/luci/recipes-py/commit/0e4d4e97edb04696b0b1452522eb26f1dc6... |
