The other benefit of adding retcode is that it allows us to better implement the following logic:

"""
    if self.add_json_log is True or (
        self.add_json_log == 'on_failure' and presentation.status != 'SUCCESS'):
      if valid:
        with contextlib.closing(recipe_util.StringListIO()) as listio:
          json.dump(ret, listio, indent=2, sort_keys=True)
        presentation.logs[self.label] = listio.lines
      else:
        presentation.logs[self.label + ' (invalid)'] = raw_data.splitlines()
        presentation.logs[self.label + ' (exception)'] = (
          invalid_error.splitlines())
"""

which relies on the assumption that presentation.status [which gets automatically set based on the retcode and ok_ret] is accurate. This in turn relies on the assumption that the code is not using ok_ret. Some code [e.g. the code in the CL mentioned in the opener] uses ok_ret and then relies on the results of the OutputPlaceholder to accurately set the presentation.status.

I'm not sure if I understand why exactly you need the retcode in the above code? Shouldn't we respect what the step determines about its own success of failure, as determined by the combination of retcode and ok_ret? 

Could you rewrite the code snippet in #2 to use retcode instead? So I can see what you want things to look like after you plumb the return code through everything.

+ iannucci, perhaps you could shed some light on this.

I think that there's some confusion about the intended purpose of ok_ret. Functionally, ok_ret does two things:
  * Determines whether a given retcode [e.g. 1] causes an exception to be thrown.
  * Updates presentation.status with 'SUCCESS', 'FAILURE', or 'EXCEPTION'

See: 
https://cs.chromium.org/chromium/infra/recipes-py/recipe_engine/run.py?l=236
https://cs.chromium.org/chromium/infra/recipes-py/recipe_engine/types.py?l=181

As per discussion on https://chromium-review.googlesource.com/c/chromium/tools/build/+/1234173/, one of the take aways was that if a step is expected to sometimes have retcode = 1 [semantically meaning failure], then it's common to set: ok_ret=(0, 1), and then to manually have the caller update presentation.status.

This means that there's a brief period of time where presentation.status is potentially incorrect [in this case, it will have 'SUCCESS', rather than 'FAILURE']. 

Furthermore, the implementation of OutputPlaceholder currently uses presentation.status:
https://cs.chromium.org/chromium/infra/recipes-py/recipe_modules/json/api.py?l=70

My current plan is to pass retcode to OutputPlaceholder so that it can stop using presentation.status [which is potentially inaccurate]:
https://chromium-review.googlesource.com/c/infra/luci/recipes-py/+/1238733

Is this the right approach?


> Could you rewrite the code snippet in #2 to use retcode instead?
"""
    if self.add_json_log is True or (
        self.add_json_log == 'on_failure' and retcode != 0 ):
      if valid:
"""
Here, we rely on the assumption taht retcode != 0 means failure, rather than assuming that presentation.status is up to date and accurate. presentation.status is currently computed only using retcode, which makes it impossible to distinguish between FAILURE and EXCEPTION.

Yeah I think passing through the retcode is the only viable way to do this.... however what if you had a tool which legitimately returns e.g. 0 or 88 on success (some buildbot scripts do/have done this), but still want to only show the json log on other failures?

I think a better solution might be to separate the ok_ret functionality into two pieces; one piece to determine step status, and the other to raise or not.

The problem stems from the status of the step being calculated in multiple places, right (once by ok_ret and again by the user recipe code)? Ideally (I think) the flow would go:

  run the step
  read/process the outputs (placeholders)
  calculate the status (red/green/etc.)
  adjust the presentation (text, logs, etc.)
  raise an exception (if needed)

Instead of having the 'result' function on placeholders both process the outputs and adjust the presentation, maybe it should be split into two functions, one to read, and another to adjust presentation (using the calculated status)

Had a nice long discussion with iannucci. 

Summary: Given that none of the OutputPlaceholders currently need retcode, and that WebRTC has a workaround to have tests always emit output, even on success, we should just leave things as is. 

The fact that the OutputPlaceholder currently uses presentation.status to emit logs is actually itself an artifact of problems with the recipe-engine/user-recipes [python 32-bit running out of memory for really large error messages]. 

This is likely doing the wrong thing, sometimes, since presentation.status may not be accurate. If no one notices, we can keep things as is. If someone does notice, we can try changing JsonOutputPlaceholder to always emit logs, regardless of presentation.status, and hope that that doesn't blow up the recipe engine.

As an aside: If we can move all state processing out of the OutputPlaceholder, then everything gets simpler again. With a small refactor we could change TestResultsPlaceholderOutput to not be a placeholder output -- instead we could just post process the json.output in the user recipe and extract the relevant results there. That's probably the best long term approach but I don't intend to change anything unless an issue comes up.