Skip to content
This repository was archived by the owner on Mar 13, 2022. It is now read-only.

Remove all shebangs from Python modules and checker #155

Merged
merged 1 commit into from
Aug 29, 2019
Merged

Remove all shebangs from Python modules and checker #155

merged 1 commit into from
Aug 29, 2019

Conversation

oz123
Copy link
Contributor

@oz123 oz123 commented Aug 20, 2019 •
edited
Loading

As discussed, Python modules which aren't intended to be invoked
as scripts should not include a shebang line.

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 20, 2019
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 20, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 20, 2019
@micw523
Copy link
Contributor

micw523 commented Aug 20, 2019

/assign

@oz123 oz123 mentioned this pull request Aug 25, 2019
Closed
@yliaog
Copy link
Contributor

yliaog commented Aug 26, 2019

Travis CI failing, could you please take a look?

@micw523
Copy link
Contributor

micw523 commented Aug 26, 2019

Gotta change the check for the boilerplate test as well @oz123

@micw523
Copy link
Contributor

micw523 commented Aug 26, 2019

This file here https://github.com/kubernetes-client/python-base/blob/master/hack/boilerplate/boilerplate.py.txt

@oz123
Copy link
Contributor Author

oz123 commented Aug 26, 2019 •
edited
Loading

This file here https://github.com/kubernetes-client/python-base/blob/master/hack/boilerplate/boilerplate.py.txt

The file should still be there. But I forgot to remove the shebang line from some modules, so now the test is failing.
Along the way, we now guarantee that no one add a shebang where it's not necessary, and if it is necessary - as in the script hack/boilerplate/boilerplate.py one can add it to the list of SKIP_FILES in this script.

@yliaog @micw523 all tests pass now.

@micw523
Copy link
Contributor

micw523 commented Aug 26, 2019

/lgtm

Although I still worry a little bit about the files that are supposed to contain the shebang (I believe there is not any in this repo right now).

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2019
@micw523
Copy link
Contributor

micw523 commented Aug 26, 2019

PS: This needs a tide label or a commit squash

@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
@oz123
Copy link
Contributor Author

oz123 commented Aug 27, 2019

@micw523 I pushed new changes documenting how to handle scripts that should include a shebang.
This is addresses your concern about this.
I also squashed all the commits so now this PR needs a new approval.

@codecov-io
Copy link

codecov-io commented Aug 27, 2019 •
edited
Loading

Codecov Report

Merging #155 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #155   +/-   ##
=======================================
  Coverage   93.56%   93.56%           
=======================================
  Files          13       13           
  Lines        1384     1384           
=======================================
  Hits         1295     1295           
  Misses         89       89
Impacted Files Coverage Δ
config/exec_provider_test.py 98.38% <ø> (ø) ⬆️
config/incluster_config.py 85.1% <ø> (ø) ⬆️
watch/watch.py 100% <ø> (ø) ⬆️
watch/watch_test.py 98.7% <ø> (ø) ⬆️
watch/__init__.py 100% <ø> (ø) ⬆️
config/exec_provider.py 82.6% <ø> (ø) ⬆️
config/kube_config.py 88.15% <ø> (ø) ⬆️
config/dateutil.py 95.83% <ø> (ø) ⬆️
config/incluster_config_test.py 97.29% <ø> (ø) ⬆️
config/__init__.py 100% <ø> (ø) ⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b8e89f...c941d74. Read the comment docs.

micw523
micw523 reviewed Aug 27, 2019
View reviewed changes
@oz123
Remove all shebangs from Python modules and checker
c941d74 
As discussed, Python modules which aren't intended to be invoked
as scripts should not include a shebang line.

Update CONTRIBUTING.md and the checker script.
This script now includes a list SKIP_FILES for files that
should not be checked for boilerplate template.
The tests will now fail if a Python module has a shebang line.
Scripts which should have a shebang line and exists in the directory
`hack` can be ignored by adding them to the SKIP_FILES list.
@micw523
Copy link
Contributor

micw523 commented Aug 27, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
@yliaog
Copy link
Contributor

yliaog commented Aug 29, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oz123, yliaog

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019
@k8s-ci-robot k8s-ci-robot merged commit 4c1ab55 into kubernetes-client:master Aug 29, 2019
@oz123 oz123 deleted the remove-shebangs branch October 2, 2019 18:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@micw523 micw523 micw523 left review comments

@mbohlool mbohlool Awaiting requested review from mbohlool

@roycaihw roycaihw Awaiting requested review from roycaihw

Assignees

@micw523 micw523

@yliaog yliaog

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oz123 @micw523 @yliaog @codecov-io @k8s-ci-robot