Allow bots with RUN_TIME flash devices#5251
Conversation
jardondiego
left a comment
jardondiego
left a comment
There was a problem hiding this comment.
Looks good, overall. :)
Left a small question there.
Xeicker
force-pushed
the
flash-pixels
branch
2 times, most recently
from
59096c7 to
f0d3282
Compare
|
|
||
| run_timeout = environment.get_value('RUN_TIMEOUT') | ||
| if run_timeout: | ||
| force_flashing = str(environment.get_value('FORCE_ANDROID_FLASHING', 'false')).lower() == 'true' |
There was a problem hiding this comment.
If I recall correctly, there is a utility function utils.string_is_true that already does this. I guess my next question would be, if we think of this envvar as boolean, I believe environment.get_value as is would return True directly.
There was a problem hiding this comment.
nit: should we name this as SHOULD_FORCE_ANDROID_FLASHING?
There was a problem hiding this comment.
I updated the code to use string is true, looks a lot better.
About the name if it is not too bad it would be great to keep it as is. I already, landed the change for it in the script that setups that env var
|
It seems we have failing tests. |
There is an assumption that id RUN_TIME is defined then flashing android devices is not needed, however, that's not always true. This opens the possibility to allow flashing even if RUN_TIME is defined Signed-off-by: Edgar Aguilar <eledgar@google.com>
It is not failing, it is pending waiting for a collaborator to give the command |
I just relaunched the CI jobs. Overall changes LGTM |
| force_flashing = utils.string_is_true( | ||
| environment.get_value('FORCE_ANDROID_FLASHING')) |
There was a problem hiding this comment.
| force_flashing = utils.string_is_true( | |
| environment.get_value('FORCE_ANDROID_FLASHING')) | |
| force_flashing = environment.get_value('FORCE_ANDROID_FLASHING') |
letitz
left a comment
letitz
left a comment
There was a problem hiding this comment.
LGTM % two things:
- One comment in the code about a log
- You misspelled
RUN_TIMEOUTasRUN_TIMEin the PR title and description
| environment.get_value('FORCE_ANDROID_FLASHING')) | ||
|
|
||
| if run_timeout and not force_flashing: | ||
| logs.info('Skipping reimage: RUN_TIMEOUT is set.') |
There was a problem hiding this comment.
Let's mention FORCE_ANDROID_FLASHING too.
|
/gcbrun |
4 participants
Description
There is an assumption that id
RUN_TIMEis defined then flashing android devices is not needed, however, that's not always true.This opens the possibility to allow flashing even if
RUN_TIMEis definedTesting