test: fixing most tests for v2-config-only true #3743
test: fixing most tests for v2-config-only true #3743alyssawilk merged 4 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@envoyproxy/maintainers for your review. I'm not sending an envoy-announce email quite yet - I'm curious if folks think we should land the tests separately (in case of rollback) and/or delay until Monday (to optimize work days for anyone whose pipelines get caught by this). Once we've agreed on a plan I'll draft something for the announce list. Thoughts? |
| @@ -0,0 +1,818 @@ | |||
| { | |||
There was a problem hiding this comment.
Should we convert these to YAML since they are checked in files that will be used by folks? I wouldn't suggest doing it by hand, but maybe some CLI or Web-based JSON->YAML converter.
There was a problem hiding this comment.
Sure. It doesn't automatically convert because of all the {{}}s but I swapped them out, converted, and then (I think) swapped them back correctly. Oh and a bunch more updates to the scripts which want to parse yaml. Now they read yaml and split out json because dumping yaml caused some weird parsing error and I think this is v2 enough for now. :-)
I think given this isn't going to get consensus to land this morning, and next week is a holiday week for folks in the states I'm going to make this a test-only change, email envoy-announce, and flag flip on the 9th. Description updated accordingly.
|
Ohhh, I forgot next week is special. I'd say land ASAP or wait until the 9th then |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing this.
|
"ImportError: No module named yaml" |
|
And argh again - docker build fails with another round of #1816. I'll go and fix that too. Meanwhile htuch@ suggests that we shouldn't actually require pyyaml for our tests to run but should instead us virtualenv. I'm really not excited (for me and everyone else) to run virtualenv and install pyyaml every time the hot restart test is run. I'm inclined to instead go back to the json version and leave the yaml files checked in as reference config, [edit] or maybe just do sed instead of parsed yaml editing. Thoughts? offline: Harvey apparently really prefers yaml. I really prefer not slowing down all the tests. Hand-parsing? |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
changed the title
| finally: | ||
| admin_conn.close() | ||
| else: | ||
| raw_yaml = original_file.readlines() |
There was a problem hiding this comment.
Sorry why do we need to do this? Can't we use a venv with some better parsing stuff?
There was a problem hiding this comment.
Is there a compromise where we don't need any venv but just do a couple of very dumb regexes?
There was a problem hiding this comment.
We can do a venev, but this script is called under blaze test //test/integration:all. I think spinning up a virtual environment and installing yaml is way too heavyweight to do on every single test run.
Beyond being slow and expensive it'd also make tests fail if you're running them locally (with no network connectivity). I really don't think it's the way to go, but htuch@ feels as strongly that we shouldn't add yaml to the docker build image because of second order effects with version conflicts and didn't like the original json verison either. Options as they stand are
- venv (alyssa dislikes)
- add yaml to our docker image (htuch dislikes)
- use json config for this test (htuch dislikes)
- parse yaml manually (matt appears to dislike)
Surely we can find some compromise we all dislike evenly? :-D
@htuch this was the version where I didn't venv and I do dumb regex. I just forgot to remove the yaml import (now fixed)
There was a problem hiding this comment.
The reason not to add the yaml package to the Docker environment is that this forces every developer who builds natively to put this in their Python system library. We really don't want any non-hermetic dependencies. You could try make it a Bazel native dependency, but I suspect that will be more effort than it's worth, unless @jmillikin-stripe could jump in and explain how to do it.
I'm surprised how complicated the "dumb regex" method is; shouldn't it be just a few lines?
There was a problem hiding this comment.
not when we're replacing 7 addresses and I'm writing the python.
There was a problem hiding this comment.
OK, I think this code is just duct tape test infra anyway, I don't feel that strongly about making it super clean. I think potentially more verbose code than the minimal needed is less worse than using JSON as example human readable configs, so ship it.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
3 participants
Risk Level: n/a
Testing: updated the 2 lingering tests to better pass with v2.
Docs Changes: n/a
Release Notes: n/a