Assert validity of stream parameters.

Assert validity of stream parameters.

Assert that stream parameters are valid. A lot of these validity checks
are related to limitations in annotations (e.g., no newlines), but
others are actually content errors. This adds assertions that:

1) No annotation parameter may include a newline in it. This is
   disallowed because the annotation parser cannot handle multi-line
   single annotations.
2) Build properties must be valid JSON. This is a requirement imposed by
   the annotation parser (chromium_step.py), and will lead to total
   waterfall breakages if invalid JSON is exported.

This also adds unit tests to check that these assertions work.

BUG=None
TEST=unit

Review-Url: https://codereview.chromium.org/2792333003
Committed: https://github.com/luci/recipes-py/commit/ddfc94f01e4e1d3c7bf7dedfd58e6813bc5160df

[dnj, 2017-04-04 18:14:14]

dnj@chromium.org changed reviewers:
+ iannucci@chromium.org, martiniss@chromium.org

[dnj, 2017-04-04 18:14:15]

PTAL

[dnj, 2017-04-04 18:16:02]

The CQ bit was checked by dnj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:16:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 18:19:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-04 18:19:19]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-04-05 02:40:24]

lgtm, thanks for doing this!

https://codereview.chromium.org/2792333003/diff/1/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2792333003/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:347: assert '\n' not in url, 'Link URL must not contain
a newline.'
Since these values are user-controlled (i.e. they vary at runtime), it would be
better not to use `assert` for them (since -O removes asserts in cpython:
`python -O -c 'assert False'` exits 0).

Would you mind switching the asserts in this file to a function (maybe
"check()"?) that raises ValueError instead?

https://codereview.chromium.org/2792333003/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:360: json.loads(value) # value must be a valud JSON
object.
nit: valud

[tandrii(chromium), 2017-04-05 12:23:52]

tandrii@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2017-04-05 12:23:53]

general question: won't this break some badly written recipes that currently
work, be it not always with good UI?

[dnj, 2017-04-05 15:07:36]

On 2017/04/05 12:23:53, tandrii(chromium) wrote:
> general question: won't this break some badly written recipes that currently
> work, be it not always with good UI?

It could, yeah. Most of these assumptions would have either manifested as
something broken (e.g., master crashes b/c of invalid property name) earlier or
as missing information (e.g., broken step links).

That said, expectation generation will fail first, preventing a roll, so only
cases where expectations don't match reality are really vulnerable to unexpected
breakages. Additionally, if things really break in production, the recipe was
already broken and needs to be fixed. These assertions should have always been
in place.

We don't really have a good way to roll out a soft check of this and actually
gather response information. We could turn builds red or whatever, but that
still requires a human to catch the problem.

[dnj, 2017-04-05 15:08:07]

https://codereview.chromium.org/2792333003/diff/1/recipe_engine/stream.py
File recipe_engine/stream.py (right):

https://codereview.chromium.org/2792333003/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:347: assert '\n' not in url, 'Link URL must not contain
a newline.'
On 2017/04/05 02:40:23, iannucci wrote:
> Since these values are user-controlled (i.e. they vary at runtime), it would
be
> better not to use `assert` for them (since -O removes asserts in cpython:
> `python -O -c 'assert False'` exits 0).
> 
> Would you mind switching the asserts in this file to a function (maybe
> "check()"?) that raises ValueError instead?

Done.

https://codereview.chromium.org/2792333003/diff/1/recipe_engine/stream.py#new...
recipe_engine/stream.py:360: json.loads(value) # value must be a valud JSON
object.
On 2017/04/05 02:40:23, iannucci wrote:
> nit: valud

Done.

[tandrii(chromium), 2017-04-05 15:13:33]

On 2017/04/05 15:07:36, dnj wrote:
> On 2017/04/05 12:23:53, tandrii(chromium) wrote:
> > general question: won't this break some badly written recipes that currently
> > work, be it not always with good UI?
> 
> It could, yeah. Most of these assumptions would have either manifested as
> something broken (e.g., master crashes b/c of invalid property name) earlier
or
> as missing information (e.g., broken step links).
> 
> That said, expectation generation will fail first, preventing a roll, so only
> cases where expectations don't match reality are really vulnerable to
unexpected
> breakages. Additionally, if things really break in production, the recipe was
> already broken and needs to be fixed. These assertions should have always been
> in place.
> 
> We don't really have a good way to roll out a soft check of this and actually
> gather response information. We could turn builds red or whatever, but that
> still requires a human to catch the problem.

I see. While I agree with you, I think landing this CL warrants a PSA as we
*could* be breaking something that sort of worked for our customers before.

[dnj, 2017-04-05 15:14:15]

On 2017/04/05 15:13:33, tandrii(chromium) wrote:
> On 2017/04/05 15:07:36, dnj wrote:
> > On 2017/04/05 12:23:53, tandrii(chromium) wrote:
> > > general question: won't this break some badly written recipes that
currently
> > > work, be it not always with good UI?
> > 
> > It could, yeah. Most of these assumptions would have either manifested as
> > something broken (e.g., master crashes b/c of invalid property name) earlier
> or
> > as missing information (e.g., broken step links).
> > 
> > That said, expectation generation will fail first, preventing a roll, so
only
> > cases where expectations don't match reality are really vulnerable to
> unexpected
> > breakages. Additionally, if things really break in production, the recipe
was
> > already broken and needs to be fixed. These assertions should have always
been
> > in place.
> > 
> > We don't really have a good way to roll out a soft check of this and
actually
> > gather response information. We could turn builds red or whatever, but that
> > still requires a human to catch the problem.
> 
> I see. While I agree with you, I think landing this CL warrants a PSA as we
> *could* be breaking something that sort of worked for our customers before.

Yes, I was planning on landing this alongside a PSA.

[iannucci, 2017-04-06 22:07:03]

lgtm

[dnj, 2017-04-06 22:11:03]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-04-06 22:11:03]

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/2792333003/#ps60001
(title: "fix spelling")

[commit-bot: I haz the power, 2017-04-06 22:11:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 22:14:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491516663351610,
"parent_rev": "0160aa0a9b2b85379a8e80090d753fef20e6dce5", "commit_rev":
"ddfc94f01e4e1d3c7bf7dedfd58e6813bc5160df"}

[commit-bot: I haz the power, 2017-04-06 22:14:24]

Description was changed from

==========
Assert validity of stream parameters.

Assert that stream parameters are valid. A lot of these validity checks
are related to limitations in annotations (e.g., no newlines), but
others are actually content errors. This adds assertions that:

1) No annotation parameter may include a newline in it. This is
   disallowed because the annotation parser cannot handle multi-line
   single annotations.
2) Build properties must be valid JSON. This is a requirement imposed by
   the annotation parser (chromium_step.py), and will lead to total
   waterfall breakages if invalid JSON is exported.

This also adds unit tests to check that these assertions work.

BUG=None
TEST=unit
==========

to

==========
Assert validity of stream parameters.

Assert that stream parameters are valid. A lot of these validity checks
are related to limitations in annotations (e.g., no newlines), but
others are actually content errors. This adds assertions that:

1) No annotation parameter may include a newline in it. This is
   disallowed because the annotation parser cannot handle multi-line
   single annotations.
2) Build properties must be valid JSON. This is a requirement imposed by
   the annotation parser (chromium_step.py), and will lead to total
   waterfall breakages if invalid JSON is exported.

This also adds unit tests to check that these assertions work.

BUG=None
TEST=unit

Review-Url: https://codereview.chromium.org/2792333003
Committed:
https://github.com/luci/recipes-py/commit/ddfc94f01e4e1d3c7bf7dedfd58e6813bc5...
==========

[commit-bot: I haz the power, 2017-04-06 22:14:25]

Committed patchset #4 (id:60001) as
https://github.com/luci/recipes-py/commit/ddfc94f01e4e1d3c7bf7dedfd58e6813bc5...

[Paweł Hajdan Jr., 2017-04-07 09:03:54]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2798223005/ by phajdan.jr@chromium.org.

The reason for reverting is: Breaks downstream testing in tools/build repo.

Both simulation_test and test hang, and throw exceptions, e.g.:

ValueError: Failed assertion: Step text must not contain a newline. {'text':
'<br/>\n===== PERF TRY JOB RESULTS =====\n\nTest Command:
src/tools/perf/run_benchmark -v --browser=android-chrome
--output-format=chartjson sunspider --upload-bucket=public\nTest Metric:
dummy/dummy\nRelative Change: 88.88889%\nStandard Error: +- 0.00000
delta\n\nRevision    Mean        Std.Error\nPatch       1.0         0.0\nNo
Patch    9.0         0.0\n<br/>'}.