Skip to content
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

ci: add pylint job #2

Merged
merged 7 commits into from
Nov 28, 2022
Merged

Conversation

aesteve-rh
Copy link
Member

Add pylint.yml to have basic linting CI job.

Signed-off-by: Albert Esteve aesteve@redhat.com

@aesteve-rh aesteve-rh self-assigned this Aug 30, 2022
@aesteve-rh aesteve-rh force-pushed the aesteve/ci-pylint branch 11 times, most recently from 2b6fee1 to 1410a3c Compare August 30, 2022 12:37
@aesteve-rh aesteve-rh marked this pull request as draft August 30, 2022 12:40
@aesteve-rh
Copy link
Member Author

Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

I did not review all changes, but they do not look helpful for this code. In general the code is worse with these changes, and it is full of cryptic pylint codes.

Most likely the pylint configuration enable rules that should not be enabled. Start with minimal configuration that pass with the current code, and add rules one by one as needed.

backup/backup.py Outdated Show resolved Hide resolved
backup/backup.py Outdated Show resolved Hide resolved
backup/backup.py Outdated Show resolved Hide resolved
backup/imagetransfer.py Outdated Show resolved Hide resolved
backup/imagetransfer.py Outdated Show resolved Hide resolved
backup/test.py Outdated Show resolved Hide resolved
backup/test.py Outdated Show resolved Hide resolved
backup/test.py Outdated Show resolved Hide resolved
backup/test.py Outdated Show resolved Hide resolved
backup/test.py Outdated Show resolved Hide resolved
@aesteve-rh
Copy link
Member Author

I have reviewed the rules and the ones that are enabled are pretty basic, and helped to fix a few bugs in the code. I'll ready this PR up.

@aesteve-rh aesteve-rh marked this pull request as ready for review September 27, 2022 12:05
@aesteve-rh
Copy link
Member Author

@aesteve-rh aesteve-rh force-pushed the aesteve/ci-pylint branch 3 times, most recently from d822a54 to ad98e6e Compare November 28, 2022 15:23
Copy link
Member

@nirs nirs left a comment

Choose a reason for hiding this comment

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

Did only quick review, it looks good.

Adding from e may be a problem if the original code expect to see the original exception.

Add pylint.yml to have basic linting CI job.
The workflow is the official GH pylint template
with some extra installs to satisfy dependencies.

This requires installing dependiencies on each run,
but is a good starting point for the new workflow.

Use an autogenerated pylintrc configuration
that passes with current code.

Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
@aesteve-rh
Copy link
Member Author

https://github.com/aesteve-rh/ovirt-stress/actions/runs/3566260851

The action passed on my fork :)

@aesteve-rh aesteve-rh merged commit ac36bff into oVirt:master Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants