Skip to content

Add autoremove, force-removal-of-dependent-packages for opkg.remove#47240

Closed
chotea wants to merge 1 commit intosaltstack:developfrom
chotea:extend_opkg_remove_support
Closed

Add autoremove, force-removal-of-dependent-packages for opkg.remove#47240
chotea wants to merge 1 commit intosaltstack:developfrom
chotea:extend_opkg_remove_support

Conversation

@chotea
Copy link
Contributor

@chotea chotea commented Apr 23, 2018

What does this PR do?

This PR adds support for opkg remove in terms of giving the possibility of specifying two extra parameters: force-removal-of-dependent-packages, autoremove

Previous Behavior

The implementation was always calling opkg remove without any extra parameters.

New Behavior

Now we check these 2 force-options in kwargs and append them to the command if necessary:

  • --force-removal-of-dependent-packages - Remove package and all dependencies
  • --autoremove - Remove packages that were installed automatically to
    satisfy dependencies

Tests written?

No

Commits signed with GPG?

No

Add extra options for opkg remove:
- --force-removal-of-dependent-packages - Remove package and all dependencies
- --autoremove - Remove packages that were installed automatically to
satisfy dependencies

Signed-off-by: Cristian Hotea <cristian.hotea@ni.com>
@ghost ghost self-requested a review April 23, 2018 11:27
Copy link
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

...and adding a unit test would be a great wrap-up, alongside with the suggestions. 😉

A list of packages to delete. Must be passed as a python list. The
``name`` parameter will be ignored if this option is passed.

remove_dependent_packages
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "remove_dependencies" would be better. What else than "packages" can be possibly removed in the context of "dependency"?

cmd = ['opkg', 'remove']
if kwargs.get('remove_dependent_packages', False):
cmd.append('--force-removal-of-dependent-packages')
if kwargs.get('auto_remove', False):
Copy link
Contributor

Choose a reason for hiding this comment

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

You actually not need to write default False to .get(...). In this case perfectly enough to:

if kwargs.get('auto_remove'):
    cmd.append...

It will either return None (no param) or any value. You can validate it though, if you want to force people pass True/False instead of 1/0 etc.


.. versionadded:: Fluorine

auto_remove
Copy link
Contributor

Choose a reason for hiding this comment

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

auto_remove_deps perhaps?

@cachedout
Copy link
Contributor

@chotea Have you had a chance to review the feedback on this PR?

@rallytime
Copy link
Contributor

@chotea Any chance you were able to come back to this one?

@chotea
Copy link
Contributor Author

chotea commented Jun 4, 2018

Yes. I implemented the feedback and wrote some tests. I wasn't able to update this PR because I was shifted to something with more priority. I will come back ASAP.

@rallytime
Copy link
Contributor

@chotea Awesome! We look forward to seeing the updates. :)

@rallytime
Copy link
Contributor

Since we haven't heard back here for a while, I'm closing this PR.

We'd still love to have this change, so when you're ready to implement the feedback, let us know and we will gladly open this up again. If it's easier for you to just submit a new PR, that's fine too.

Thank you!

@rallytime rallytime closed this Jul 3, 2018
@chotea
Copy link
Contributor Author

chotea commented Jul 12, 2018

@rallytime Can you please reopen this PR? I have implemented the feedback and wrote unit tests for opkg module.

@rallytime
Copy link
Contributor

@chotea Hrm, it appears that the Reopen and comment button that is usually here is grayed out on this PR for some reason. Can you open a new PR against develop and link it to this one for feedback context?

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.

4 participants