Verify-boilerplate script #107
Conversation
k8s-ci-robot
added
cncf-cla: yes
run_tox.sh
Outdated
| @@ -1,4 +1,4 @@ | |||
| #!/bin/bash | |||
| #!/usr/bin/env python | |||
iamneha
force-pushed
the
verify-scripts
branch
from
8a220cd to
1bdcaf1
Compare
Codecov Report
@@ Coverage Diff @@
## master #107 +/- ##
=======================================
Coverage 92.04% 92.04%
=======================================
Files 13 13
Lines 1182 1182
=======================================
Hits 1088 1088
Misses 94 94
Continue to review full report at Codecov.
|
|
very cool! /assign @roycaihw /lgtm |
|
/assign @mbohlool Mehdy, Roy, this looks ready |
| @@ -1,3 +1,5 @@ | |||
| #!/usr/bin/env python | |||
|
|
|||
There was a problem hiding this comment.
I don’t think the python shebang should appear in the non-executable files. This applies to most files you added the shebang.
There was a problem hiding this comment.
@micw523 this seems to be what was in k/k main repo for boilerplate. i think this is ok - https://github.com/kubernetes/kubernetes/blob/master/hack/boilerplate/boilerplate.py.txt#L1
There was a problem hiding this comment.
@dims I agree if that’s how the main repo did things. Since I’m not very familiar with the main repo, does the main repo contain any python functions or it’s all executables with main()?
There was a problem hiding this comment.
Does the script only verify python-base files? I'd expect CI to fail since we don't have the python shebang in most of the generated files in python repo
There was a problem hiding this comment.
@roycaihw yes this script only verify python-base files.
I added ./hack/verify-boilerplate.sh inside python-base/.travis.yml
And python has different .travis.yml so I don't think it will affect the pthon repo
There was a problem hiding this comment.
@roycaihw correct me if I’m wrong, but the python files in the main repo seem to be mostly executables?
hack/verify-boilerplate.sh
Outdated
| @@ -0,0 +1,35 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| # Copyright 2014 The Kubernetes Authors. | |||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(main()) |
There was a problem hiding this comment.
Please add this file to the list of files for pycodestyle checking
There was a problem hiding this comment.
I'll add this directory in kubernetes-client/python/script once this PR is merged
There was a problem hiding this comment.
I’m fine with it but please run autopep8 with aggressive level 2 on your file to check for the coding style to see if there’s any changes.
There was a problem hiding this comment.
@micw523 i think we should leave it as a simple copy of the code in main k/k repository. If we need to do more, we should do there first and copy it over here.
iamneha
force-pushed
the
verify-scripts
branch
from
1bdcaf1 to
d56fdbc
Compare
|
/lgtm |
|
This is generally looks good to me. However, I didn't get the point about python-base vs the main python repo. If this is only run for the python-base, I couldn't see why in 2 minutes I spend on this. I defer the review decision to @roycaihw as he had the same comment. |
|
@mbohlool let's start here and we can even reuse in the other repo. for me the motivation was that there were at least 3 files which had some bits in the license header wrong |
|
/approve I think verifying python-base on its own is helpful |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamneha, roycaihw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Fixes issue kubernetes-client/python#22