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

pacman: re-adding support for URL based pkgs #4286

Merged
merged 7 commits into from
Mar 1, 2022
Merged

pacman: re-adding support for URL based pkgs #4286

merged 7 commits into from
Mar 1, 2022

Conversation

jraby
Copy link
Contributor

@jraby jraby commented Feb 25, 2022 •
edited
Loading

SUMMARY

Fixes #4285

consider this a draft, it still lacks test cases for URL based packages.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

pacman

@jraby jraby mentioned this pull request Feb 25, 2022
Closed
1 task
@ansibullbot
Copy link
Collaborator

cc @indrajitr @tchernomax
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug has_issue module module os packaging plugins plugin (any type) tests tests unit tests/unit labels Feb 25, 2022
@jraby
Copy link
Contributor Author

jraby commented Feb 26, 2022

Added test and fixed a few rough edges.

With this, using URL/filenames in both state=present and state=absent works.
It is possible to mix regular packages and URL based ones, as shown in the test.

I'm not super happy about how the tests look now, but I think a refactoring for this can wait.

@jraby jraby force-pushed the 4285-broken-local-file-support branch from 7801ee2 to ab569a0 Compare February 26, 2022 16:14
@jraby
Copy link
Contributor Author

jraby commented Feb 26, 2022

@felixfontein I think this is ready for review now.

If you have ideas on how to rework the code for easier testing without as much boilerplate, I'm all ears.

@felixfontein felixfontein added backport-4 check-before-release PR will be looked at again shortly before release and merged if possible. labels Feb 27, 2022
felixfontein
felixfontein reviewed Feb 27, 2022
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Two improvements (these can help debugging). Besides that, LGTM.

plugins/modules/packaging/os/pacman.py Outdated Show resolved Hide resolved
plugins/modules/packaging/os/pacman.py Outdated Show resolved Hide resolved
@jraby jraby force-pushed the 4285-broken-local-file-support branch from 1f2f199 to e7f0ab7 Compare February 28, 2022 00:42
@jraby jraby mentioned this pull request Feb 28, 2022
Closed
jraby added 2 commits February 28, 2022 10:42
@jraby
Add more tests + fix minor bug with URL packages
8da322e 
Version checking for URL packages is left to pacman, so add a check
after the dry run to see if it would actually install anything.
@jraby
remove double templating
c5189cc
@jraby
Copy link
Contributor Author

jraby commented Feb 28, 2022

As mentioned in #4298 , the last few commits add integration tests for URL based package installation.
(as well as fix a bug with check mode vs URL pkgs)

@ansibullbot ansibullbot added the integration tests/integration label Feb 28, 2022
@felixfontein felixfontein merged commit a9db474 into ansible-collections:main Mar 1, 2022
@patchback
Copy link

patchback bot commented Mar 1, 2022 •
edited
Loading

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/a9db4742fc44e74dea48ea7952bbe504acd0b410/pr-4286

Backported as #4302

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Mar 1, 2022
@jraby @patchback
pacman: re-adding support for URL based pkgs (#4286)
68f9d6f 
* pacman: re-adding support for URL based pkgs

* Update plugins/modules/packaging/os/pacman.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/modules/packaging/os/pacman.py

Co-authored-by: Felix Fontein <[email protected]>

* cmd=cmd in every call to self.fail()

* pacman: integration test for mixed pkg sources

* Add more tests + fix minor bug with URL packages

Version checking for URL packages is left to pacman, so add a check
after the dry run to see if it would actually install anything.

* remove double templating

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit a9db474)
@patchback patchback bot mentioned this pull request Mar 1, 2022
Merged
@felixfontein
Copy link
Collaborator

@jraby thanks for fixing this and adding more tests :)

@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Mar 1, 2022
felixfontein pushed a commit that referenced this pull request Mar 1, 2022
@patchback @jraby
pacman: re-adding support for URL based pkgs (#4286) (#4302)
391c3aa 
* pacman: re-adding support for URL based pkgs

* Update plugins/modules/packaging/os/pacman.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/modules/packaging/os/pacman.py

Co-authored-by: Felix Fontein <[email protected]>

* cmd=cmd in every call to self.fail()

* pacman: integration test for mixed pkg sources

* Add more tests + fix minor bug with URL packages

Version checking for URL packages is left to pacman, so add a check
after the dry run to see if it would actually install anything.

* remove double templating

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit a9db474)

Co-authored-by: Jean Raby <[email protected]>
@jraby
Copy link
Contributor Author

jraby commented Mar 1, 2022

@felixfontein thanks for setting up the integration tests stuff!

@felixfontein
Copy link
Collaborator

@jraby yw! Feel free to add more tests ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug has_issue integration tests/integration module module os packaging plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pacman: Can't install from local file
3 participants
@jraby @ansibullbot @felixfontein