Verify-boilerplate script by iamneha · Pull Request #107 · kubernetes-client/python-base

iamneha · 2018-12-03T11:03:31Z

dims · 2018-12-03T12:06:48Z

@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env python


This is a bash script :)

codecov-io · 2018-12-03T14:19:34Z

Codecov Report

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

@@           Coverage Diff           @@
##           master     #107   +/-   ##
=======================================
  Coverage   92.04%   92.04%           
=======================================
  Files          13       13           
  Lines        1182     1182           
=======================================
  Hits         1088     1088           
  Misses         94       94

Impacted Files	Coverage Δ
config/config_exception.py	`100% <ø> (ø)`	⬆️
config/exec_provider_test.py	`98.38% <ø> (ø)`	⬆️
config/incluster_config.py	`85.1% <ø> (ø)`	⬆️
watch/watch.py	`100% <ø> (ø)`	⬆️
watch/watch_test.py	`98.13% <ø> (ø)`	⬆️
config/kube_config_test.py	`94.41% <ø> (ø)`	⬆️
watch/__init__.py	`100% <ø> (ø)`	⬆️
config/exec_provider.py	`82.6% <ø> (ø)`	⬆️
config/kube_config.py	`84.24% <ø> (ø)`	⬆️
config/incluster_config_test.py	`97.29% <ø> (ø)`	⬆️
... 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 879ab01...1bdcaf1. Read the comment docs.

dims · 2018-12-03T14:25:53Z

very cool!

/assign @roycaihw

/lgtm

dims · 2018-12-04T14:56:29Z

/assign @mbohlool

Mehdy, Roy, this looks ready

micw523

This generally looks good!

I left a few comments. I’m on vacation so I may not reply very quickly.

micw523 · 2018-12-04T23:55:45Z

@@ -1,3 +1,5 @@
+#!/usr/bin/env python
+


I don’t think the python shebang should appear in the non-executable files. This applies to most files you added the shebang.

@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

@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()?

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

@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

@roycaihw correct me if I’m wrong, but the python files in the main repo seem to be mostly executables?

micw523 · 2018-12-05T00:00:52Z

@@ -0,0 +1,35 @@
+#!/usr/bin/env bash
+
+# Copyright 2014 The Kubernetes Authors.


It’s now 2018 ;)

+1 to fix up the date

micw523 · 2018-12-05T00:04:41Z

+
+
+if __name__ == "__main__":
+    sys.exit(main())


Please add this file to the list of files for pycodestyle checking

I'll add this directory in kubernetes-client/python/script once this PR is merged

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.

@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.

dims · 2018-12-06T11:58:02Z

/lgtm

mbohlool · 2018-12-16T23:42:07Z

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.

dims · 2018-12-17T14:53:55Z

@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

roycaihw · 2019-01-23T02:47:21Z

/approve

I think verifying python-base on its own is helpful

k8s-ci-robot · 2019-01-23T02:47:24Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [roycaihw]

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 requested review from mbohlool and yliaog December 3, 2018 11:03

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 3, 2018

iamneha changed the title ~~Verify scripts~~ Verify-boilerplate script Dec 3, 2018

k8s-ci-robot requested a review from dims December 3, 2018 11:05

dims reviewed Dec 3, 2018

View reviewed changes

Comment thread run_tox.sh Outdated

@@ -1,4 +1,4 @@

#!/bin/bash

#!/usr/bin/env python

dims Dec 3, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bash script :)

iamneha force-pushed the verify-scripts branch from 8a220cd to 1bdcaf1 Compare December 3, 2018 14:10

k8s-ci-robot assigned dims and roycaihw Dec 3, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2018

k8s-ci-robot assigned mbohlool Dec 4, 2018

micw523 suggested changes Dec 5, 2018

View reviewed changes

iamneha added 2 commits December 5, 2018 22:22

Add verify-boilerplate script

86ae2de

Verify Boilerplate fix

d56fdbc

iamneha force-pushed the verify-scripts branch from 1bdcaf1 to d56fdbc Compare December 5, 2018 16:53

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2018

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019

k8s-ci-robot merged commit 8497dfb into kubernetes-client:master Jan 23, 2019

iamneha mentioned this pull request Mar 8, 2019

REQUEST: New membership for Neha kubernetes/org#587

Closed

6 tasks done

micw523 mentioned this pull request Aug 15, 2019

Boilerplate includes un-necessary shebang line #154

Closed

		@@ -0,0 +1,35 @@
		#!/usr/bin/env bash

		# Copyright 2014 The Kubernetes Authors.

Conversation

iamneha commented Dec 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Dec 3, 2018

Codecov Report

Uh oh!

dims commented Dec 3, 2018

Uh oh!

dims commented Dec 4, 2018

Uh oh!

micw523 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamneha Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dims commented Dec 6, 2018

Uh oh!

mbohlool commented Dec 16, 2018

Uh oh!

dims commented Dec 17, 2018

Uh oh!

roycaihw commented Jan 23, 2019

Uh oh!

k8s-ci-robot commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

iamneha Dec 5, 2018 •

edited

Loading