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

Relax requirements.txt restrictions #11199

Merged

Conversation

itayporezky
Copy link
Contributor

@itayporezky itayporezky commented May 15, 2024

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

  • The requirements.txt was filled with legacy packages which are no longer needed.
  • Reduces maintenance(Less dependabot PRs)
  • Reduces breaking changes chance
  • Reduces potential security risks
  • Increases simplicity

Special notes for your reviewer:
Each commit has a specific message describing why the specific package was removed. please review on per-commit basis.

Does this PR introduce a user-facing change?:

Reduced required python packages in requirements.txt

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 15, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @itayporezky. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 15, 2024
@ErikJiang
Copy link
Member

ErikJiang commented May 16, 2024

That looks good, but I am inclined to consolidate all the commits in this pull request into one. WDYT? Also, please pay attention to the CI errors related to Ansible lint, as it appears to be related to the Jinja dependency.

@VannTen
Copy link
Contributor

VannTen commented May 16, 2024

If we go that route, there is two things we should do:

  • have a more fine-grained ansible dependency, meaning depending on ansible-core instead of ansible and only adding the collection we need (see galaxy.yml)
  • optionnaly, switch to a lockfile scheme (with hash) to have more reproducible builds.
    This has bitten us in the past where the same version of Ansible installed by the same person at different time had different bugs :/

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2024
@itayporezky itayporezky force-pushed the relax-requirementstxt branch from f81a482 to a95eb05 Compare May 16, 2024 15:39
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2024
@itayporezky
Copy link
Contributor Author

If we go that route, there is two things we should do:

* have a more fine-grained ansible dependency, meaning depending on ansible-core instead of ansible and only adding the collection we need (see galaxy.yml)

* optionnaly, switch to a lockfile scheme (with hash) to have more reproducible builds.
  This has bitten us in the past where the same version of Ansible installed by the same person at different time had different bugs :/
  1. This sounds kinda risky, vast majority install ansible via ansible and not ansible-core, many people wont install dependencies manually from galaxy.yaml IMO
  2. Running pip compile on the requirements.txt in the PR will result in this:
    I'm not sure how would you maintain this file, how do you prevent dependabot from creating PR on dependencies and who would generate this compilation and update the requirements once the time comes?
# This file was autogenerated by uv via the following command:
#    uv pip compile requirements.txt
ansible==9.5.1
    # via -r requirements.txt
ansible-core==2.16.6
    # via
    #   -r requirements.txt
    #   ansible
attrs==23.2.0
    # via
    #   -r requirements.txt
    #   jsonschema
    #   referencing
cffi==1.16.0
    # via
    #   -r requirements.txt
    #   cryptography
cryptography==42.0.7
    # via
    #   -r requirements.txt
    #   ansible-core
jinja2==3.1.4
    # via
    #   -r requirements.txt
    #   ansible-core
jsonschema==4.22.0
    # via -r requirements.txt
jsonschema-specifications==2023.12.1
    # via
    #   -r requirements.txt
    #   jsonschema
markupsafe==2.1.5
    # via
    #   -r requirements.txt
    #   jinja2
packaging==24.0
    # via
    #   -r requirements.txt
    #   ansible-core
pycparser==2.22
    # via
    #   -r requirements.txt
    #   cffi
pyyaml==6.0.1
    # via
    #   -r requirements.txt
    #   ansible-core
referencing==0.35.1
    # via
    #   -r requirements.txt
    #   jsonschema
    #   jsonschema-specifications
resolvelib==1.0.1
    # via
    #   -r requirements.txt
    #   ansible-core
rpds-py==0.18.1
    # via
    #   -r requirements.txt
    #   jsonschema
    #   referencing

@yankay
Copy link
Member

yankay commented May 17, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2024
@VannTen
Copy link
Contributor

VannTen commented May 17, 2024 via email

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 19, 2024
@itayporezky
Copy link
Contributor Author

That looks good, but I am inclined to consolidate all the commits in this pull request into one. WDYT? Also, please pay attention to the CI errors related to Ansible lint, as it appears to be related to the Jinja dependency.

Please re-test,
No problem to consolidate commits, I just did it this way for the review.

@VannTen
Copy link
Contributor

VannTen commented May 19, 2024 via email

@itayporezky
Copy link
Contributor Author

I'm not sure why the CI failed

@VannTen
Copy link
Contributor

VannTen commented May 19, 2024 via email

@itayporezky itayporezky force-pushed the relax-requirementstxt branch from 57d5f99 to 9b3462d Compare May 19, 2024 19:47
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed do-not-merge/contains-merge-commits size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 19, 2024
@itayporezky
Copy link
Contributor Author

Hmm.. i'm sorry, i might have rebased wrongly.
I simply updated my fork, git pull, git rebase origin/master to my branch
is it not the way?

@itayporezky itayporezky force-pushed the relax-requirementstxt branch from 9b3462d to fd476fd Compare May 20, 2024 18:38
@itayporezky
Copy link
Contributor Author

Not sure what is going on with pre-commit CI..

@VannTen
Copy link
Contributor

VannTen commented May 21, 2024 via email

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2024
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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-sigs/prow repository.

@itayporezky itayporezky force-pushed the relax-requirementstxt branch from 6692420 to 809ea57 Compare May 30, 2024 14:31
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 30, 2024
@itayporezky
Copy link
Contributor Author

I've rebased after pre-commit fixes, i'm not sure why the current errors happen. it doesn't seem to be related to my PR IMO.

@VannTen
Copy link
Contributor

VannTen commented May 30, 2024 via email

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 30, 2024
@ant31
Copy link
Contributor

ant31 commented May 31, 2024

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ant31, itayporezky

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2024
@k8s-ci-robot k8s-ci-robot merged commit 351393e into kubernetes-sigs:master May 31, 2024
77 checks passed
@Cedran-Bastien
Copy link

Cedran-Bastien commented May 31, 2024

Hello
'ruamel' module is required by the "contrib/inventory_builder/inventory.py" file for inventory generation
I can't use it and obtain this error since this commit.
image

@itayporezky
Copy link
Contributor Author

Hello 'ruamel' module is required by the "contrib/inventory_builder/inventory.py" file for inventory generation I can't use it and obtain this error since this commit. image

contrib/inventory_builder has its own requirements at contrib/inventory_builder/requirements.txt

ehsan310 pushed a commit to ehsan310/kubespray that referenced this pull request May 31, 2024
@Cedran-Bastien
Copy link

Oh okay thanks !

Rickkwa pushed a commit to Rickkwa/kubespray that referenced this pull request Jun 26, 2024
@yankay yankay mentioned this pull request Aug 28, 2024
@ruant
Copy link

ruant commented Sep 27, 2024

@itayporezky
I know this is already merged, but:

You comment: #11199 (comment)

contrib/inventory_builder has its own requirements at contrib/inventory_builder/requirements.txt

I don't see this mentioned anywhere in the docs?
Using the premade docker image my first thought is that is should just come with all the needed dependencies installed?
Since the Dockerfile only installs the https://github.com/kubernetes-sigs/kubespray/blob/master/requirements.txt and not the contrib/inventory_builder/requirements.txt.

Perhaps the Dockerfile should be updated to install these dependencies too?

@VannTen
Copy link
Contributor

VannTen commented Sep 27, 2024 via email

davidumea pushed a commit to elastisys/kubespray that referenced this pull request Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants