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.

Sign up for GitHub

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

Jump to bottom

various CI improvements #472

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

various CI improvements #472

wants to merge 6 commits into from
+97 −46

Conversation

a-dubs
Copy link
Collaborator

@a-dubs a-dubs commented Feb 20, 2025 •
edited
Loading

Description

  • Add PR template to repository to make PRs more standardized and easier to review (this PR description is using it!)
  • replaced tox pytest env in default list with new py38 env that explicitly runs pytest using python 3.8 to ensure that code being tested does not introduce anything incompatible with our minimum supported version (3.8).
  • replaced pylint with ruff's pylint implementation because the pylint env would take up to 30s, whereas the ruff check only takes a few seconds. this makes running CI locally much more enjoyable.

Additional Context and Relevant Issues

Test Steps

@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch from b2b01f6 to 81c23aa Compare February 20, 2025 15:22
@a-dubs a-dubs changed the title Pycloudlib typing and capabilities overhaul Add networking types to standardize networking configs and implement for oracle Feb 20, 2025
@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch 4 times, most recently from 115f158 to 1112501 Compare February 21, 2025 20:56
@a-dubs a-dubs changed the title Add networking types to standardize networking configs and implement for oracle Pycloudlib v11.0.0 - Allow for keeping snpashots, add new types and move to dedicated typing module Feb 21, 2025
@a-dubs a-dubs changed the title Pycloudlib v11.0.0 - Allow for keeping snpashots, add new types and move to dedicated typing module Pycloudlib v11.0.0 - Allow for keeping snapshots, add new types and move them to dedicated typing module Feb 21, 2025
@a-dubs a-dubs mentioned this pull request Feb 21, 2025
Closed
6 tasks
@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch 3 times, most recently from 0525a14 to e22c4c7 Compare February 24, 2025 15:58
@codyshepherd
Copy link

Can you describe which changes you consider breaking and which are not?

@codyshepherd
Copy link

I understand now that the feat! commits are considered breaking. Is it possible to separate those out from the other changes to make a smaller PR?

@a-dubs
Copy link
Collaborator Author

a-dubs commented Feb 24, 2025

thanks for the feedback @codyshepherd. I will remove the breaking change commits from this PR and separate those into a new set of PRs against a new v11 branch.

@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch from e22c4c7 to 4ce75b3 Compare February 24, 2025 16:41
@a-dubs a-dubs changed the title Pycloudlib v11.0.0 - Allow for keeping snapshots, add new types and move them to dedicated typing module various CI improvements Feb 24, 2025
@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch from 4ce75b3 to d4c1cd1 Compare February 24, 2025 16:42
codyshepherd
codyshepherd requested changes Feb 24, 2025
View reviewed changes
Copy link

@codyshepherd codyshepherd left a comment

Choose a reason for hiding this comment

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

Can you help me understand how/where ruff's pylint has been subbed for traditional pylint? I see that the pylint command has been removed from the tox.ini line 66, is the ruff check command in line 78/84 already doing the ruff linting, and this PR just disabled the duplicate effort?

tox.ini Outdated
@@ -1,14 +1,13 @@
[tox]
# As it may be undesired to make formatting changes, by default only check
envlist = ruff, format-check, mypy, pytest, pylint
envlist = ruff, format-check, mypy, py38
Copy link

Choose a reason for hiding this comment

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

Why not include py310 and py312 in this list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I felt it was somewhat redundant since 3.10 and 3.12 will be backwards compatible with 3.8 and didn't want to run essentially redundant tests, but to be fair the tests only take a handful of seconds to run so I would be happy to add py310 and py12 as default envs as well!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After trying this, the GH focal runners fail to run py310 and py312 so I think for now it would be best to leave py38 as the only one in the tox default env list

Copy link

Choose a reason for hiding this comment

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

Shall we just remove 3.10 and 3.12 then? what's the value of keeping them in if they aren't run?

a-dubs
a-dubs commented Feb 24, 2025
View reviewed changes
@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch 3 times, most recently from 47b53f1 to eed5c17 Compare February 26, 2025 19:19
@a-dubs
Copy link
Collaborator Author

a-dubs commented Feb 26, 2025

@codyshepherd I have responded to all of your comments and also added a new commit that creates dedicated py38, py310, and py312 github action jobs for PRs to address your comment.

Ready for re-review!

@a-dubs a-dubs requested a review from codyshepherd February 26, 2025 19:26
@a-dubs
feat(github): add pull request template
82b6c8f
@a-dubs
ci(tox): replace pytest default env with py38, py310, and py312
1a2e526 
By adding a py38 env that runs the same commands as the pytest env but
runs on Python 3.8, we can catch errors against our minimum supported
python version. Only py38 is included in the default envlist to make
local CI quicker.
@a-dubs
ci: remove pylint and use ruff pylint rules instead
a901087 
This replaces pylint with the ruff pylint rules for conventions (PLC),
errors (PLE), and warnings (PLW). The pylint refactoring rules (PLR)
were left out since they are extraneous, and as the name implies,
require heavy refactoring. See Ruff's pylint rules for more info. [1]

Also, bumped ruff version from 0.4.4 to 0.5.0

Refs:
[1] https://docs.astral.sh/ruff/rules/#pylint-pl
@a-dubs
feat(tox): remove redundant format-check env and properly use posargs
7d13073 
Allow for calling tox -e mypy and tox -e ruff with custom pos args
(args following "--"). Also remove the format-check tox env and combine
it with the existing `ruff` env.
@a-dubs
ci(github): split out pytest runs into version specific jobs
3b45434 
This adds 3 new github actions jobs that will run on PRs: py38, py310,
and py312. Now the pytest jobs and the linting jobs are separate.
@a-dubs a-dubs force-pushed the pycloudlib-typing-and-capabilities-overhaul branch from eed5c17 to 3b45434 Compare February 26, 2025 19:41
uhryniuk
uhryniuk approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@uhryniuk uhryniuk left a comment

Choose a reason for hiding this comment

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

Looks like you addressed most of the comments, keep an eye after this gets merged to make sure the CI doesn't explode.

codyshepherd
codyshepherd requested changes Feb 26, 2025
View reviewed changes
tox.ini

[testenv:format]
envdir = {[common]envdir}
deps = {[common]deps}
commands =
{envpython} -m ruff format -- .
{envpython} -m ruff check --fix -- pycloudlib examples setup.py
{envpython} -m ruff format {posargs:pycloudlib examples setup.py}
Copy link

Choose a reason for hiding this comment

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

Will this command change the formatting in place? i.e. will it make edits to the code if there are edits to be had?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, as is standard for most of our projects, the format env will fix linting issues and do formatting in place. for this project, the ruff command will run the same commands but will only check and will not make any changes.

@a-dubs a-dubs requested a review from codyshepherd February 27, 2025 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpocase rpocase rpocase left review comments

@uhryniuk uhryniuk uhryniuk approved these changes

@codyshepherd codyshepherd Awaiting requested review from codyshepherd

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@a-dubs @codyshepherd @rpocase @uhryniuk