-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
USHIFT-1030: fix pylint finding issues #1614
Changes from 10 commits
7e1d39a
201d5f2
66f81bf
75c7f8e
285edd7
13bfc60
018ced9
c3a0333
1aa57ef
651db57
0c5f410
9c5bdf9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
#!/bin/bash | ||
|
||
ROOTDIR=$(git rev-parse --show-toplevel) | ||
REQ_FILE=${ROOTDIR}/requirements.txt | ||
VENV="/tmp/venv" | ||
PYLINT="pylint" | ||
|
||
if ! command -v ${PYLINT} &>/dev/null; then | ||
|
||
check="$(python3 -m pip install -r ${REQ_FILE} 2>&1 | { grep 'Permission denied' || true; })" | ||
|
||
# Install pylint in a virtual environment for CI | ||
if [ ! -z "$check" ] ; then | ||
printf "Installing pylint in '${VENV}' virtual environment" | ||
python3 -m venv ${VENV} | ||
${VENV}/bin/python3 -m pip install --upgrade pip | ||
${VENV}/bin/python3 -m pip install -r ${REQ_FILE} | ||
PYLINT="/tmp/venv/bin/pylint" | ||
fi | ||
fi | ||
|
||
PYFILES=$(find . -type d \( -path ./_output -o -path ./vendor -o -path ./assets -o -path ./etcd/vendor \) -prune -o -name '*.py' -print) | ||
printf "Running ${PYLINT} for \n${PYFILES}\n" | ||
|
||
${PYLINT} --variable-naming-style=any ${PYFILES} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
PyGithub==1.56 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Over time, especially as we set up e2e tests, we are likely to have different requirements files for different purposes. This one should probably move to hack as something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am unsure what is the recommended way, however we need all the dependencies installed before running pylint. We may have to loop over all the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, moved the |
||
GitPython==3.1.18 | ||
PyYAML==3.12 | ||
tabulate==0.8.10 | ||
tqdm==4.64.1 | ||
jira==3.2.0 | ||
pylint==2.13.9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just use the virtualenv all the time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pylint installation worked fine directly in RHEL dev VM, without virtualenv. And as virtualenv will put a little overhead in the VM, I think we should do that only for CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on a fence with this one, I'd like to have the same flow in CI and locally whenever possible.
Do we need all the packages listed in
requirements.txt
for pylint? (I'd move that file toscripts/auto-rebase/
)If pylint can run without all packages, then what about:
venv --symlinks
on my local system it's 33mb vs 23mb_output/
, so consecutive runs (in local VM) don't have to redo the venvWhat do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need all the packages listed in
requirements.txt
for pylint to run successfully, otherwise it will throw(import-error)
.For having venv locally, we could setup that in
_output/
folder and install all the dependencies there. In that case, we should run all python screipts from that venv, and install further dependencies there only. We should not have two copies of dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in Slack, we may later on, create a separate PR to run all .py scripts from venv rather than requiring to have deps installed on system (usually /usr/, and not $HOME...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really recommended practice to use pip to install things directly into the system python space, so I would prefer we use virtualenv all the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've updated the script to create a virtual environment all the time.