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

Adds the possibility to also use downloadonly in kwargs #54793

Closed
wants to merge 9 commits into from
Closed

Adds the possibility to also use downloadonly in kwargs #54793

wants to merge 9 commits into from

Conversation

brejoc
Copy link
Contributor

@brejoc brejoc commented Sep 27, 2019

What does this PR do?

The download_only parameter in the apt module is not in line with the yum and zypper modules. Both of them use downloadonly without the underline.

With this change apt now additionally supports the downloadonly parameter.

What issues does this PR fix or reference?

Fixes #54790

Previous Behavior

The apt module had to be used with download_only.

New Behavior

The apt module can now also be used with downloadonly.

Tests written?

Yes

Commits signed with GPG?

Yes

The download_only parameter in the apt module is not in line with
the yum and zypper modules. Both of them use downloadonly without
the underline.

With this change apt now additionally supports the downloadonly
parameter.

Fixes #54790
@brejoc
Copy link
Contributor Author

brejoc commented Sep 27, 2019

I'll add tests once I know that we'd like to take this route.
Tests are added.

@brejoc brejoc marked this pull request as ready for review September 27, 2019 14:46
@cmcmarrow
Copy link
Contributor

Thanks you for the test and keeping backwards compatibility in mind.

@dwoz
Copy link
Contributor

dwoz commented Sep 29, 2019

@brejoc The unit.modules.test_aptpkg.AptPkgTestCase.test_upgrade_downloadonly test is failing.

@brejoc
Copy link
Contributor Author

brejoc commented Oct 11, 2019

This is really strange. Locally the tests are passing and I can see the download-only parameter in the args list. But when run here, the args are just empty:

=============================================================

()
{}
=============================================================

()
{}

@brejoc
Copy link
Contributor Author

brejoc commented Oct 29, 2019

@dwoz looks like this is good to go now. Would it make sense to rebase to master?

@brejoc
Copy link
Contributor Author

brejoc commented Nov 6, 2019

@saltstack/core-pr-reviewers Would it make more sense to close this and open an other PR with these changes against master?

@brejoc
Copy link
Contributor Author

brejoc commented Nov 27, 2019

@dwoz Since this was closed, I opened the same for master: #55448

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.

3 participants