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

Remove un-needed function #68482

Merged
merged 14 commits into from
Apr 8, 2020
Merged

Remove un-needed function #68482

merged 14 commits into from
Apr 8, 2020

Conversation

ChrisAHolland
Copy link
Contributor

SUMMARY

Comments explaining this function state:

# FIXME: Remove this once this PR is merged:
#   https://github.com/ansible/ansible/pull/19172

And it so appears that the PR has been merged some time ago, so let's reduce unnecessary code!

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

modules/packaging/os/dnf.py

mkrizek
mkrizek requested changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@mkrizek mkrizek left a comment

Choose a reason for hiding this comment

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

That method is still used:

ansible/lib/ansible/modules/packaging/os/dnf.py

Line 795 in c61f141

name = self.fetch_rpm_from_url(name)

Please see the pull request you linked on how the method was replaced in other packaging modules.

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. packaging Packaging category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 26, 2020
@ansibot
Copy link
Contributor

ansibot commented Mar 26, 2020

@ChrisAHolland this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added owner_pr This PR is made by the module's maintainer. merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Mar 26, 2020
@ChrisAHolland
Copy link
Contributor Author

Everything should be good now!

@ansibot ansibot removed the owner_pr This PR is made by the module's maintainer. label Mar 26, 2020
@mkrizek
Copy link
Contributor

mkrizek commented Mar 27, 2020

With the method removed there are now some unused imports which should be removed as well.

@mkrizek
Copy link
Contributor

mkrizek commented Mar 27, 2020

@ChrisAHolland this PR contains the following merge commits:

* [1bb78cb](https://github.com/ansible/ansible/commit/1bb78cba2d17e4080dfbe268975fd46726bc5d5c)

Please rebase your branch to remove these commits.

click here for bot help

This needs to be resolved as well.

@ChrisAHolland
Copy link
Contributor Author

With the method removed there are now some unused imports which should be removed as well.

I removed the 1 import for fetch_url. That was the only one I believe.

@ansibot ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Mar 27, 2020
@mkrizek
Copy link
Contributor

mkrizek commented Mar 30, 2020

Also tempfile is unused now.

@ChrisAHolland
Copy link
Contributor Author

Also tempfile is unused now.

Now removed.

@mkrizek
Copy link
Contributor

mkrizek commented Mar 31, 2020

Code looks good to me now.

@mkrizek mkrizek requested a review from maxamillion April 1, 2020 06:46
@ChrisAHolland
Copy link
Contributor Author

ChrisAHolland commented Apr 2, 2020 •
edited

@mkrizek @maxamillion Any updates? I'm excited to have my first PR merged :) Thanks!

@ChrisAHolland
Copy link
Contributor Author

@mattclay @bcoca Could I get a review and merge on this PR? Sorry for being annoying :P

@mkrizek
Copy link
Contributor

mkrizek commented Apr 8, 2020 •
edited

I have missed this earlier but the lines 309 and 310 should be removed as well as the constant is now unused:

# 64k.  Number of bytes to read at a time when manually downloading pkgs via a url
BUFSIZE = 65536

@ChrisAHolland
Copy link
Contributor Author

@mkrizek Made the changes! I appreciate your help!

@mkrizek mkrizek merged commit c9c1146 into ansible:devel Apr 8, 2020
@ChrisAHolland
Copy link
Contributor Author

Thanks for the help and merge!

@ansible ansible locked and limited conversation to collaborators May 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattclay mattclay mattclay approved these changes

@mkrizek mkrizek mkrizek approved these changes

@maxamillion maxamillion Awaiting requested review from maxamillion

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. packaging Packaging category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ChrisAHolland @ansibot @mkrizek @mattclay