Refactor python code: commonlize scripts utils and bootstrap #437
Conversation
|
/ok-to-test |
Codecov Report
@@ Coverage Diff @@
## master #437 +/- ##
=========================================
Coverage ? 74.58%
=========================================
Files ? 34
Lines ? 3463
Branches ? 0
=========================================
Hits ? 2583
Misses ? 697
Partials ? 183
Continue to review full report at Codecov.
|
|
rebased PR and adjusted content |
|
The shared library is good, but don't mess with the logging messages. Leave the logging up to each library as the DaisyRun* prefix is not at all descriptive of the various tasks that we have. Also its not matching what we do for other workflows. |
Alright, I agree, however it'll be tricky to unify a bootstrap.py with such a requirement. Maybe I can make that prefix a parameter. Let's see. |
|
@adjackura done. Please review once again |
|
wait, I'll actually take this chance and also remove the gsutil dependency of bootstrap by copying the files from the bucket using a different mechanism. |
Any hints? |
And in order to do that, I intended to use the most up-to-date / robust flavor of a functionality found. Took this chance also to solve some flake8 warnings that I could.
That will assist supporting distros that doesn't have gsutil installed.
gut
force-pushed
the
refactor-python-code
branch
from
e12a426
to
79acd23
Compare
|
Done. Now bootstrap doesn't depend on gsutils anymore, which I believe it's a win. (changes separated on the 2nd commit) |
|
This change looks good but since it has a lot of changes and I may have missed something, I'll test our image builds after it is merged as well. On removing gsutil, do that in another PR. You would want to use the cloud storage python library and make sure you use the default service account credentials from the VM. |
No problem, I can remove the commit from this PR, but I'm confused as this PR is approved. Will it be eventually merged or are you expecting me to make this changes before?
I'm not sure every image would have that package installed, would it? I preferred to use the base REST API for implementing it as I it would require less dependencies. Nevertheless it'd be even easier and simpler if I used the cloud storage python library. Shall I proceed with all changes then? |
|
Oh maybe I missed something? I still saw gsutil being used and called in this PR: |
Interesting. That's not bootstrap.py but I agree it makes sense to change it to a more pythonic way :-). I can do ti on a different PR, no problem. I'll just wait for this one as refactoring always freezes development so it would be better to make that PR on top of this one. |
|
Oh I see- you were just talking about the bootstrap code- ok cool. I think we should probably get rid of the gsutil dependency everywhere if possible however that can be done later. Thanks! Can you sync your branch so I can merge. |
|
@gut: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
done |
And in order to do that, I intended to use the most up-to-date / robust
flavor of a functionality found.
Some output now is controlled through DaisyRunSuccess, DaisyRunStatus
and DaisyRunFailed.
Took this chance also to solve some flake8 warnings that I could.