tools: updating deprecation scripts#6289
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
https://pastebin.com/ahycWwku for sample email |
|
Sorry for crummy diffs - only like 1/3 of this is new but because I stuck code in a function all the diffs are different. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
htuch
left a comment
There was a problem hiding this comment.
LGTM, but do you think we should start adding tests, even if it's a simple golden text file style, as this script is becoming more involved?
|
|
||
|
|
||
| # Gather code and suggested email changes. | ||
| (runtime_email, runtime_features_code) = flip_runtime_features() |
There was a problem hiding this comment.
Nit: parens aren't needed here in tuple destruction.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Golden files won't work here, as the output would change every time we ran the deprecation script. I think I could refactor the functions so the grep were function input, then we could test the functions with echo commands faking what should happen. I got as far as trying to figure out how to run an envoy python test, realized we don't do that, realized I'd need to run an sh test in some sort of venv wrapper and convert error codes, and came back to "there is no way I'd rather do this than just fix the tool once a quarter if it were broken" |
|
@alyssawilk I was mostly after adding tests if it was reasonable, no need to do gymnastics. We can revisit if the script continues to grow more complicated. Will review later today. |
htuch
left a comment
There was a problem hiding this comment.
LGTM; a few nits, feel free to fix and merge.
| for (filename, field) in filenames_and_fields: | ||
| code += (' "envoy.deprecated_features.' + filename + ':' + field + '",\n') | ||
| email += (field + ' from ' + filename + '\n') | ||
| return (email, code) |
There was a problem hiding this comment.
Nit: no need for parens here or the other return statements.
| code = '' | ||
| email = 'the following features will be defaulted to true:\n' | ||
| for (feature) in features_to_flip: | ||
| code += (' "' + feature + '",\n') |
There was a problem hiding this comment.
Nit: prefer building up a list and then doing a join later.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
Sadly the way github works I will need a follow-up stamp |
| email = '' | ||
| if email_snippets: | ||
| email = ('\nThe following deprecated configuration fields will be disallowed by default:\n' + | ||
| ''.join(email_snippets)) |
There was a problem hiding this comment.
Looks good, but one slight Pythonic nit. Could we do '\n'.join(email_snippets)'? That way the above append becomes a bit more declarative. Same for code_snippets (and below..).
There was a problem hiding this comment.
I can, but I'd prefer not to if I can talk you out of it :-)
If I pull newlines out of email snippets I think I should do it for the code for consistency. But for code it's a need-to-have and for email it's a nice-to-have to have the trailing \n after the join. I mean I could append another newline after both but if I want it after each snippet rather than between each snippet I think having it in the snippet rather than having it in the join is semantically correct.
There was a problem hiding this comment.
It's semantically correct, I'm just suggesting there are more Pythonic ways to do it if you're after Python style suggestions. Since the join value is substituted into a string in both cases, you can write something like 'foo bar:\n %s\n' % '\n'.join(stuff without any new lines in it). This discussion is really me being nit-picky, but reflects my understanding of Python best practices.
Updating per new file locations. Updates (unused) reloadable flags to default true.
Risk Level: n/a (tooling)
Testing: manual
Docs Changes: n/a
Release Notes: n/a