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

Run pre-commit hooks to fix python test imports #3221

Merged
merged 2 commits into from Nov 8, 2022
Merged

Conversation

lucacome
Copy link
Member

@lucacome lucacome commented Nov 4, 2022

  • Removes unused imports in tests
  • Adds fixtures and utils folders to move non-tests files from the main folder
  • Re-orders dependencies in the requirements files alphabetically
  • Fixes whitespaces in a few remaining files

@lucacome lucacome self-assigned this Nov 4, 2022
@lucacome lucacome requested a review from a team as a code owner November 4, 2022 00:18
@github-actions github-actions bot added chore Pull requests for routine tasks documentation Pull requests/issues for documentation tests Pull requests that update tests labels Nov 4, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2022

Codecov Report

Merging #3221 (4291b16) into main (fb6bbeb) will increase coverage by 0.03%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##             main    #3221      +/-   ##
==========================================
+ Coverage   52.52%   52.56%   +0.03%     
==========================================
  Files          58       59       +1     
  Lines       16070    16080      +10     
==========================================
+ Hits         8441     8452      +11     
  Misses       7351     7351              
+ Partials      278      277       -1     
Impacted Files Coverage Δ
internal/certmanager/cm_controller.go 18.04% <ø> (ø)
internal/certmanager/helper.go 100.00% <ø> (ø)
internal/certmanager/sync.go 73.23% <ø> (ø)
internal/configs/version2/template_helper.go 70.00% <70.00%> (ø)
internal/configs/version2/template_executor.go 60.46% <100.00%> (ø)
internal/k8s/configuration.go 95.76% <0.00%> (+0.36%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lucacome lucacome force-pushed the chore/run-pre-commit branch 2 times, most recently from 3f1ad19 to 2dd6440 Compare November 4, 2022 07:09
Copy link
Contributor

@tomasohaodha tomasohaodha left a comment

Choose a reason for hiding this comment

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

Impossible to review a PR with 511 files changed with just 'Fixes all the formatting in the codebase' as the description. I would suggest either package it to have fewer files per review, or explain in-depth what is being changed so that anomalies can be easily detected (although even then, 511 files is a lot).

@haywoodsh
Copy link
Contributor

Hi @lucacome, should there also be corresponding checks in the CI pipeline for the hooks you added? From what I understand about pre-commit hooks, users can skip them if they choose to. If the same tests don't exist in the CI pipeline, could it be possible that a pull request is merged without passing those commit hooks, and the next developer who pull from the main branch will have have to fix the code themselves?

@lucacome
Copy link
Member Author

lucacome commented Nov 4, 2022

@haywoodsh yes there's going to be a check. I've enabled it briefly but it was trying to fix all the files at once instead of fixing only the modified files. I haven't found a way yet to customize the app, so I've disabled it for now.
This PR is basically taking care of all the existing issues so that we can enable the check.

@lucacome lucacome marked this pull request as draft November 4, 2022 19:35
@lucacome
Copy link
Member Author

lucacome commented Nov 5, 2022

@tomasohaodha I've opened #3223 to move some of the docs changes there

@tomasohaodha
Copy link
Contributor

It's still showing all the whitespace-changed files. I assume you need to merge #3223 and then run the pre-commit again?

@github-actions github-actions bot removed the documentation Pull requests/issues for documentation label Nov 7, 2022
@lucacome
Copy link
Member Author

lucacome commented Nov 7, 2022

@tomasohaodha yes just merged main into this branch and also created another PR to take care of whitespaces in tests #3226

@lucacome lucacome changed the title Run pre-commit hooks on all files Run pre-commit hooks to fix python test imports Nov 7, 2022
@lucacome lucacome marked this pull request as ready for review November 7, 2022 20:09
@lucacome lucacome requested a review from vepatel November 7, 2022 20:09
@lucacome lucacome merged commit 1396149 into main Nov 8, 2022
@lucacome lucacome deleted the chore/run-pre-commit branch November 8, 2022 17:06
coolbry95 pushed a commit to coolbry95/kubernetes-ingress that referenced this pull request Nov 18, 2022
* Run pre-commit hooks to fix python test imports

* Add context.minimum_version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Pull requests for routine tasks tests Pull requests that update tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants