USHIFT-1030: fix pylint finding issues #1614

chiragkyal · 2023-04-03T15:25:15Z

Which issue(s) this PR addresses:

https://issues.redhat.com/browse/USHIFT-1030

This PR will

Add hack/verify-py.sh script to create a virtual python environment in _output/venv and install all python dependency packages in that environment and run pylint.
- If pylint is already installed, it will run against all .py files.
Add scripts/requirements.txt, which lists all the dependency packages
Fix pylint finding issues and disabled a few rules
Add make verify-py in the verify CI job

openshift-ci-robot · 2023-04-03T15:25:19Z

@chiragkyal: This pull request references USHIFT-1031 which is a valid jira issue.

In response to this:

Which issue(s) this PR addresses:

https://issues.redhat.com/browse/USHIFT-1030

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2023-04-03T15:25:28Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-04-03T15:27:06Z

@chiragkyal: This pull request references USHIFT-1030 which is a valid jira issue.

In response to this:

Which issue(s) this PR addresses:

https://issues.redhat.com/browse/USHIFT-1030

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

chiragkyal · 2023-04-03T15:29:38Z

/jira refresh

openshift-ci-robot · 2023-04-03T15:29:40Z

@chiragkyal: This pull request references USHIFT-1030 which is a valid jira issue.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dhellmann

Most of these changes look good. I had a couple of questions but nothing jumps out at me as problematic.

Some of these scripts are dev tools and are not tested in CI, so I wonder if we should apply the same approach we said we would for the bash linter? That would give us a chance to test some of them individually in case we miss an issue with visual inspection.

scripts/auto-rebase/handle-assets.py

scripts/auto-rebase/rebase.py

scripts/jira/cloner.py

scripts/auto-rebase/rebase.py

scripts/auto-rebase/handle-assets.py

scripts/release-notes/gen-ec-release-notes.py

chiragkyal · 2023-04-04T11:18:57Z

Apart from these, there are a lot of

C0301: Line too long (135/100) (line-too-long)
W1203: Use lazy % formatting in logging functions (logging-fstring-interpolation)

I think we can snooze them, because it may impact code readability with not so valuable code changes.

chiragkyal · 2023-04-04T11:42:16Z

Some of these scripts are dev tools and are not tested in CI, so I wonder if we should apply the same approach we said we would for the bash linter? That would give us a chance to test some of them individually in case we miss an issue with visual inspection.

For sure, so if I'm not wrong, the plan would be to merge one file at a time.

scripts/auto-rebase/handle-assets.py

pmtk · 2023-04-04T12:28:03Z

scripts/auto-rebase/handle-assets.py

@@ -19,18 +26,23 @@


 def merge_paths(pathl, pathr):
+    """Merge two paths into one by removing any '/' prefix from pathr and then appending it to pathl"""


Not exactly.
If pathr starts with / it means it's absolute, so we discard that leading / and return what's left.
If it's relative, then we return pathl/pathr.

Oh, yes, I misread the code again. :-(

Changed this to :

""" Merge two paths depending upon following conditions: - If `pathr` is absolute (starts with `/`), then discard the leading `/` and return rest `pathr`. - If `pathr` is relative, then return `pathl/pathr`. """

scripts/auto-rebase/handle-assets.py

pmtk · 2023-04-04T12:36:09Z

scripts/auto-rebase/handle-assets.py



 def main():
+    """Main function for handling assets based on the recipe file."""


I see that this docstring ends with . but it doesn't seem to be the case for other docstrings. Do we need to be consistent? Does pep8 or other code style guidelines say anything about this?

Checked a few examples in PEP 257 – Docstring Conventions, and most of them end with . So, I've added . for all other docstring to be consistent

scripts/auto-rebase/handle-assets.py

pmtk · 2023-04-04T12:54:15Z

scripts/auto-rebase/rebase.py

+    """
+    Posts a comment on a GitHub pull request.
+    If there are any extra messages in the global `_extra_msgs` list,
+    they will be appended to the comment.


Actually they're not appended to a comment - a new comment is created with contents of _extra_msgs

Changed to

""" Posts a comment on a GitHub pull request with the contents of the global `_extra_msgs` list. """

scripts/auto-rebase/rebase.py

scripts/jira/cloner.py

pmtk · 2023-04-04T13:27:50Z

scripts/jira/cloner.py


-    actions = []
+    actions_list = []


query_str = JQL_FILTER_QUERY and this line makes me want to ignore the rule... This reminds me of C language code style

C0103 is a very broad check for all types of naming conventions; I think we should keep it.

The main reason for this change is because actions and query are conflicting with the local variables defined in other functions. We need to either change in main function or in all other functions where it has been used.

pmtk · 2023-04-04T14:05:40Z

scripts/jira/cloner.py

    for i in tqdm(range(len(query))):
        try:
-            issue = query[i]
+            issuee = query[i]


What is reason for this change?

W0621: Redefining name 'issue' from outer scope (line 338) (redefined-outer-name)

Because issue is getting used in most of the function definitions. So, either change it in main function or in all the functions where issue is used. I chose to change in main function for simplicity and less line of change.

pmtk · 2023-04-04T14:08:08Z

Before merging this needs be tested in rebase scenario - either manually running rebase.py or creating a dummy PR in o/release to rehearse it

dhellmann · 2023-04-04T16:55:45Z

scripts/auto-rebase/handle-assets.py

@@ -19,18 +26,23 @@


 def merge_paths(pathl, pathr):
+    """Merge two paths into one by removing any '/' prefix from pathr and then appending it to pathl"""


Oh, yes, I misread the code again. :-(

scripts/auto-rebase/handle-assets.py

dhellmann · 2023-04-04T16:57:09Z

scripts/auto-rebase/rebase.py

@@ -1,8 +1,15 @@
 #!/usr/bin/env python
+# pylint: disable=fixme,global-statement,too-many-statements,too-many-locals,broad-except


Yes, let's be very careful with this file.

scripts/jira/cloner.py

scripts/release-notes/gen-ec-release-notes.py

…definition

dhellmann

I had 2 comments, but nothing to hold this up. They can be addressed in another PR, unless you want to fix them as part of addressing other comments.

I want to leave this open for @pmtk to approve, since he is more familiar with the code being changed and had previous review comments.

dhellmann · 2023-04-10T17:01:50Z

hack/verify-py.sh

+
+if ! command -v ${PYLINT} &>/dev/null; then
+
+    check="$(python3 -m pip install -r ${REQ_FILE} 2>&1 | { grep 'Permission denied' || true; })"


Why not just use the virtualenv all the time?

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.

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 to scripts/auto-rebase/)
If pylint can run without all packages, then what about:

here we just setup venv and install pylint - this will reduce the overhead

to further reduce it, we could run venv --symlinks on my local system it's 33mb vs 23mb

we could setup pylint venv in _output/, so consecutive runs (in local VM) don't have to redo the venv

What do you think?

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.

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

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.

Okay, I've updated the script to create a virtual environment all the time.

dhellmann · 2023-04-10T17:04:03Z

requirements.txt

@@ -0,0 +1,7 @@
+PyGithub==1.56


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 hack/requirements.txt so we can differentiate it from validate-microshift/requirements.txt in the future.

I think that script/auto-rebase/rebase.py and (if we decide so) e2e/tests.py should have separate requirements.txt in their own dirs. Does it make sense from how python project operate?

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 requirements.txt files if we choose to have multiple.

As discussed, moved the requirements.txt into scripts/ dir

pmtk

Let's get "venv in CI and locally" and "/requirements.txt" topics sorted out and I think we're good to go

pmtk · 2023-04-11T09:10:38Z

hack/verify-py.sh

+
+if ! command -v ${PYLINT} &>/dev/null; then
+
+    check="$(python3 -m pip install -r ${REQ_FILE} 2>&1 | { grep 'Permission denied' || true; })"


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 to scripts/auto-rebase/)
If pylint can run without all packages, then what about:

here we just setup venv and install pylint - this will reduce the overhead

to further reduce it, we could run venv --symlinks on my local system it's 33mb vs 23mb

we could setup pylint venv in _output/, so consecutive runs (in local VM) don't have to redo the venv

What do you think?

pmtk · 2023-04-11T09:16:14Z

requirements.txt

@@ -0,0 +1,7 @@
+PyGithub==1.56


I think that script/auto-rebase/rebase.py and (if we decide so) e2e/tests.py should have separate requirements.txt in their own dirs. Does it make sense from how python project operate?

pmtk · 2023-04-12T08:52:16Z

/lgtm

openshift-ci · 2023-04-12T08:52:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chiragkyal, pmtk

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:

~~OWNERS~~ [chiragkyal,pmtk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-04-12T09:16:35Z

@chiragkyal: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2023-04-12T09:19:21Z

@chiragkyal: This pull request references USHIFT-1030 which is a valid jira issue.

In response to this:

Which issue(s) this PR addresses:

https://issues.redhat.com/browse/USHIFT-1030

This PR will

Add hack/verify-py.sh script to create a virtual python environment in _output/venv and install all python dependency packages in that environment and run pylint.

If pylint is already installed, it will run against all .py files.

Add scripts/requirements.txt, which lists all the dependency packages

Fix pylint finding issues and disabled a few rules

Add make verify-py in the verify CI job

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 3, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2023

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 3, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 3, 2023

chiragkyal changed the title ~~USHIFT-1031: fix pylint finding issues~~ USHIFT-1030: fix pylint finding issues Apr 3, 2023

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 3, 2023

USHIFT-1030: fix pylint finding report

7e1d39a

chiragkyal force-pushed the fix-py-lint branch from 1a30958 to 7e1d39a Compare April 3, 2023 16:14

dhellmann reviewed Apr 3, 2023

View reviewed changes

scripts/auto-rebase/handle-assets.py Outdated Show resolved Hide resolved

scripts/auto-rebase/rebase.py Outdated Show resolved Hide resolved

USHIFT-1030:update docstring for merge_paths(pathl, pathr)

201d5f2

chiragkyal force-pushed the fix-py-lint branch from 64409ec to 201d5f2 Compare April 4, 2023 07:51

chiragkyal commented Apr 4, 2023

View reviewed changes

scripts/auto-rebase/rebase.py Outdated Show resolved Hide resolved

chiragkyal commented Apr 4, 2023

View reviewed changes

scripts/jira/cloner.py Outdated Show resolved Hide resolved

chiragkyal commented Apr 4, 2023

View reviewed changes

scripts/auto-rebase/rebase.py Show resolved Hide resolved

chiragkyal commented Apr 4, 2023

View reviewed changes

scripts/auto-rebase/handle-assets.py Outdated Show resolved Hide resolved

chiragkyal commented Apr 4, 2023

View reviewed changes

scripts/release-notes/gen-ec-release-notes.py Outdated Show resolved Hide resolved

chiragkyal marked this pull request as ready for review April 4, 2023 11:32

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2023

openshift-ci bot requested review from dhellmann and pmtk April 4, 2023 11:34

pmtk reviewed Apr 4, 2023

View reviewed changes

dhellmann reviewed Apr 4, 2023

View reviewed changes

chiragkyal added 5 commits April 5, 2023 16:44

USHIFT-1030: consistent docstring, update variable name and function …

66f81bf

…definition

USHIFT-1030: rename handle-assets.py and gen-ec-release-notes.py files

75c7f8e

USHIFT-1030: add shebang, disable 'f' logging issue

285edd7

USHIFT-1030: enable verify-py in verify CI job

13bfc60

USHIFT-1030: disable line-too-long and use pip3

018ced9

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2023

chiragkyal added 2 commits April 6, 2023 16:46

USHIFT-1030: add --variable-naming-style=any and revert a few changes

c3a0333

USHIFT-1030: Fix merge conflict

1aa57ef

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2023

chiragkyal force-pushed the fix-py-lint branch 4 times, most recently from b84f3e6 to 431046d Compare April 9, 2023 19:47

USHIFT-1030: Install pylint in virtual environment for CI

651db57

chiragkyal force-pushed the fix-py-lint branch from fbae108 to 651db57 Compare April 9, 2023 21:00

dhellmann reviewed Apr 10, 2023

View reviewed changes

pmtk reviewed Apr 11, 2023

View reviewed changes

chiragkyal added 2 commits April 11, 2023 19:50

USHIFT-1030: move requirements.txt into /scripts dir

0c5f410

USHIFT-1030: always create venv for python

9c5bdf9

openshift-ci bot assigned pmtk Apr 12, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2023

openshift-merge-robot merged commit c578570 into openshift:main Apr 12, 2023
15 checks passed

chiragkyal deleted the fix-py-lint branch April 12, 2023 09:22

		@@ -19,18 +26,23 @@


		def merge_paths(pathl, pathr):
		"""Merge two paths into one by removing any '/' prefix from pathr and then appending it to pathl"""



		def main():
		"""Main function for handling assets based on the recipe file."""

		@@ -1,8 +1,15 @@
		#!/usr/bin/env python
		# pylint: disable=fixme,global-statement,too-many-statements,too-many-locals,broad-except


		if ! command -v ${PYLINT} &>/dev/null; then

		check="$(python3 -m pip install -r ${REQ_FILE} 2>&1 \| { grep 'Permission denied' \|\| true; })"

USHIFT-1030: fix pylint finding issues #1614

USHIFT-1030: fix pylint finding issues #1614

Conversation

chiragkyal commented Apr 3, 2023 • edited

openshift-ci-robot commented Apr 3, 2023 • edited by openshift-ci bot

openshift-ci bot commented Apr 3, 2023

openshift-ci-robot commented Apr 3, 2023 • edited by openshift-ci bot

chiragkyal commented Apr 3, 2023

openshift-ci-robot commented Apr 3, 2023 • edited by openshift-ci bot

dhellmann left a comment

Choose a reason for hiding this comment

chiragkyal commented Apr 4, 2023 • edited

chiragkyal commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmtk commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiragkyal Apr 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmtk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmtk commented Apr 12, 2023

openshift-ci bot commented Apr 12, 2023

openshift-ci bot commented Apr 12, 2023

openshift-ci-robot commented Apr 12, 2023 • edited by openshift-ci bot

chiragkyal commented Apr 3, 2023 •

edited

openshift-ci-robot commented Apr 3, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Apr 3, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Apr 3, 2023 •

edited by openshift-ci bot

chiragkyal commented Apr 4, 2023 •

edited

chiragkyal Apr 11, 2023 •

edited

openshift-ci-robot commented Apr 12, 2023 •

edited by openshift-ci bot