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 --app by flatpak check if already installed (ansible-collectio… #6289

Merged
merged 4 commits into from
Apr 20, 2023
Merged

Remove --app by flatpak check if already installed (ansible-collectio… #6289

merged 4 commits into from
Apr 20, 2023

Conversation

Svenum
Copy link
Contributor

@Svenum Svenum commented Apr 4, 2023

SUMMARY

Currently, when installing a Flatpak, the command "flatpak list --[user|system] --app" is used to test whether the package is already installed or not. But since not every package is an app, packages like "org.gtk.Gtk3theme.Matcha-dark-sea" or "org.kde.KStyle.Adwaita" are not listed and are therefore not considered as installed.
Removing the "--app" option fix this.

Fixes #6265

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

flatpak module

ADDITIONAL INFORMATION

@ansibullbot
Copy link
Collaborator

cc @JayKayy @oolongbrothers
click here for bot help

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor os packaging plugins plugin (any type) small_patch Hopefully easy to review labels Apr 4, 2023
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 labels Apr 5, 2023
felixfontein
felixfontein reviewed Apr 5, 2023
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.

Thanks for your contribution! Could you please add a changelog fragment? Thanks.

@Svenum
Copy link
Contributor Author

Svenum commented Apr 5, 2023

Is done.

@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Apr 5, 2023
@ansibullbot

This comment was marked as outdated.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 5, 2023
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 5, 2023
@felixfontein
Copy link
Collaborator

Looks formally OK to me (I have no flatpak experience). If nobody objects, I'll merge this in a week or so.

@russoz
Copy link
Collaborator

russoz commented Apr 9, 2023

Same here: no flatpack experience to judge that call you're making, but looks good from the ansible perspective.

It would be nice to have integration tests for this.

@Svenum
Copy link
Contributor Author

Svenum commented Apr 9, 2023

It would be nice to have integration tests for this.

Have I to do this test and if yes, how? (Sry for this question, but its my first contribute)

@russoz
Copy link
Collaborator

russoz commented Apr 10, 2023

@Svenum not a Have To(TM) for this PR, but it would be a nice addition. It's totally OK if you prefer to defer that to a future PR.

About "How?", a quick primer:

There are two types of tests you can write as a module developer: unit-tests and integration-tests.

Unit tests are found in the test/unit/plugins directory tree and they will be usually written in Python itself using standard test frameworks for Python development (we support unittest and pytest in community.general). See some of the existing tests as examples to get some bearings. Although not a strict rule, unit tests are not expected to interact with external actors (systems, services, APIs, etc...), usually relying on mocking these actors, and ensuring that the internal behaviour is as expected. See more about unit tests.

Integration tests are found in the test/integration/targets directory and they are written (almost) as if they were Ansible roles. The module is actually put to work and it will interact with external actors. Obviously this can become complicated or costly depending on the external entity. There is no one single rule to fit all cases. We also recommend you check the existing tests as examples. See more about integration tests.

@ansibullbot ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Apr 18, 2023
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Apr 20, 2023
@felixfontein felixfontein merged commit 6e0bc4f into ansible-collections:main Apr 20, 2023
@patchback
Copy link

patchback bot commented Apr 20, 2023 •
edited
Loading

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/6e0bc4f45c4cd4a7e375f0a213c95efd7ce0da54/pr-6289

Backported as #6371

🤖 @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 Apr 20, 2023
@Svenum @patchback
Remove --app by flatpak check if already installed (ansible-collectio… (
592c010 
#6289)

* Remove --app by flatpak check if already installed (#6265)

* Add Changelogfragment

* Fix syntax

* Update changelogs/fragments/6289-bugfix-flatpak-check-if-already-installed.yml

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

---------

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 6e0bc4f)
@patchback patchback bot mentioned this pull request Apr 20, 2023
Merged
@felixfontein
Copy link
Collaborator

@Svenum thanks for your contribution!
@russoz thanks for reviewing!

@patchback
Copy link

patchback bot commented Apr 20, 2023 •
edited
Loading

Backport to stable-6: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-6/6e0bc4f45c4cd4a7e375f0a213c95efd7ce0da54/pr-6289

Backported as #6372

🤖 @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 Apr 20, 2023
@Svenum @patchback
Remove --app by flatpak check if already installed (ansible-collectio… (
e0921b0 
#6289)

* Remove --app by flatpak check if already installed (#6265)

* Add Changelogfragment

* Fix syntax

* Update changelogs/fragments/6289-bugfix-flatpak-check-if-already-installed.yml

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

---------

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 6e0bc4f)
@patchback patchback bot mentioned this pull request Apr 20, 2023
Merged
felixfontein pushed a commit that referenced this pull request Apr 20, 2023
@patchback @Svenum
[PR #6289/6e0bc4f4 backport][stable-5] Remove --app by flatpak check …
e4765f2 
…if already installed (ansible-collectio… (#6371)

Remove --app by flatpak check if already installed (ansible-collectio… (#6289)

* Remove --app by flatpak check if already installed (#6265)

* Add Changelogfragment

* Fix syntax

* Update changelogs/fragments/6289-bugfix-flatpak-check-if-already-installed.yml

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

---------

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

Co-authored-by: Svenum <[email protected]>
felixfontein pushed a commit that referenced this pull request Apr 20, 2023
@patchback @Svenum
[PR #6289/6e0bc4f4 backport][stable-6] Remove --app by flatpak check …
b10cf1e 
…if already installed (ansible-collectio… (#6372)

Remove --app by flatpak check if already installed (ansible-collectio… (#6289)

* Remove --app by flatpak check if already installed (#6265)

* Add Changelogfragment

* Fix syntax

* Update changelogs/fragments/6289-bugfix-flatpak-check-if-already-installed.yml

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

---------

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

Co-authored-by: Svenum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@felixfontein felixfontein felixfontein left review comments

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor os packaging plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

community.general.flatpak module isn't idempotent when installing packages
4 participants
@Svenum @ansibullbot @felixfontein @russoz