Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Run pylint as a part of the ESX build #739

Merged
merged 3 commits into from
Nov 17, 2016
Merged

Conversation

msterin
Copy link
Contributor

@msterin msterin commented Nov 16, 2016

Pylint (in a container) is run automatically on 'make dockerbuild' (default target) and in Drone.
If you want to run it on your box with no containers, "make pylint".

Currently it will fail the build if only there are errors (e.g. wrong arg count, etc...). It ignores all the rest (esx_service/Makefile is setting 'PYLINT_IGNORE := --errors-only -d import-error,no-name-in-module')

Going forward , we will make it more strict up to and including failing on naming convention violation, whitespaces and other PEP8 formatting exectations

testing:

  • manually introduced wrong arg counts and made sure lint fails.
  • CI

@pdhamdhere
Copy link
Contributor

Pylint (in a container) is run automatically on 'make dockerbuild' (default target) and in Drone.
If you want to run it on your box with no containers, "make pylint".

Does it mean,simple "make" on my dev box won't run pylint? IMO, it should run by default and not restricted to CI runs triggered by drone.

# Dockerfile for running pylint
#
FROM alpine
MAINTAINER "CNA Storage Team" <cnastorage@vmware.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. thanks !

@@ -0,0 +1,355 @@
[MASTER]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it tuned to check for both Python 2.x and Python 3.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2.0 and bilingual (with module 'six') it looks like pylint for py3 will require a separate container. I'll see if it can be done with Py3/bilingual support

@msterin
Copy link
Contributor Author

msterin commented Nov 16, 2016

Does it mean,simple "make" on my dev box won't run pylint? IMO, it should run by default and not restricted to CI runs triggered by drone

'make' will run pylint because 'make' runs default target (dockerbuild) and Pylint (in a container) is run automatically on 'make dockerbuild' (default target)

@msterin msterin force-pushed the runci/pylint-on-build.msterin branch from 2d0ac31 to d7c54d6 Compare November 16, 2016 21:11
Copy link
Contributor

@kerneltime kerneltime left a comment

Choose a reason for hiding this comment

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

LGTM once the Makefile comment is resolved.

@@ -68,3 +68,6 @@ endif
# redirect all
$(TARGETS):
$(MAKE) --directory=vmdk_plugin $@

.DEFAULT:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the role of this target given that we define the default target earlier in the file?

Copy link
Contributor Author

@msterin msterin Nov 17, 2016

Choose a reason for hiding this comment

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

"default target" earlier is the target which is invoked by default :-) , i.e. on make command, with no targets.
.DEFAULT: is rule set of last resort - executed when target is passed but not found in Makefile. Real good for fall through.

https://www.gnu.org/software/make/manual/html_node/Special-Targets.html says:

.DEFAULT
The recipe specified for .DEFAULT is used for any target for which no rules are found (either
explicit rules or implicit rules). See Last Resort. If a .DEFAULT recipe is specified, every file
mentioned as a prerequisite, but not as a target in a rule, will have that recipe executed on its
behalf. See Implicit Rule Search Algorithm.


CMD ["/code"]

Copy link
Contributor

Choose a reason for hiding this comment

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

Get ride of the empty lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

Mark Sterin and others added 3 commits November 17, 2016 11:07
Currently it will fail the build if there are errors (e.g. wrong arg count, etc...)
Going forward , we need to make it more strict up to and including format/PEP8
@msterin msterin force-pushed the runci/pylint-on-build.msterin branch from d7c54d6 to e8d3d0f Compare November 17, 2016 19:12
@msterin msterin merged commit 7cc07b9 into master Nov 17, 2016
@msterin msterin deleted the runci/pylint-on-build.msterin branch November 18, 2016 02:02
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants