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

Use local dummy repo for flatpak_remote integration tests #52668

Merged
merged 3 commits into from Mar 20, 2019

Conversation

Projects
None yet
4 participants
@oolongbrothers
Copy link
Contributor

oolongbrothers commented Feb 20, 2019

SUMMARY

Fixes a problem with the flatpak_remote integration tests:

They relied on the availability of the flathub repository infrastructure. This PR replaces the use of flathub with a local dummy repository.

I was not comfortable with having the integration tests enabled in the Shippable pipeline before removing this dependency, since an outage of flathub would have led to the tests failing. This PR also enables the integration tests in Shippable.

There are however some caveats, that I would need your help assessing. See below.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

flatpak_remote

ADDITIONAL INFORMATION

There are some caveats with my solution though:

The simplest way to provide the dummy repository for me was to build the test apps and repo locally and adding the resulting folder to the setup_flatpak_remote integration target. There are a few binary files in there. Nothing huge, but nevertheless some hundred K.

I think that I could generate the repo somehow. It's not completely straight-forward though, since the flatpak build process relies on runtimes hosted externally, for example on flathub. Obviously we do not want to get a dependency on flathub through the back-door here.

Question 1: Is the size of the dummy repo a problem?


Even though I could remove the dependency on flathub, there are some dependencies to external repos left. On the Ansible docker test images for Fedora and Ubuntu 18.04, we have to install the flatpak package. I guess that this is not a problem, since many integration tests rely on installing packages from distro repositories.

On Ubuntu 16.04, however, flatpak is not available, so we need to rely on the official flatpak PPA to install flatpak. We could of course ignore Ubuntu 16.04 for testing. However, testing against the flatpak PPA enables us to test against the latest version of flatpak, which increases our version coverage.

Question 2: Is the dependency on the official flatpak PPA maintained by flatpak creator Alex Larsson a problem?


I am not entirely sure as to what the different shippable posix group numbers mean.

Question3: Is shippable/posix/group3 the correct group in the pipeline? And does the shippable test run in this PR include the tests I enabled?

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 20, 2019

@oolongbrothers This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Feb 21, 2019

@oolongbrothers Thank you for taking the time to get the tests working,

What is the total size of the dummy repo?

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Feb 21, 2019

I maybe missing it, though I don't see the tests being run

https://app.shippable.com/github/ansible/ansible/runs/108979/77/console
WARNING: Excluding tests marked "needs/privileged" which require --docker-privileged to run under docker: flatpak_remote

@ansibot ansibot removed the core_review label Feb 21, 2019

@oolongbrothers

This comment has been minimized.

Copy link
Contributor Author

oolongbrothers commented Feb 21, 2019

@gundalow Happy you appreciate the testing.
The repo is about 300 kB. But when I compress it in a .tar.xz, it's only 16 kB. So I will obviously do that. Also for performance reasons, as @mattclay pointed out.
To do that, should I rather force-push the change to get rid of the old binaries or does that kill the label workflow? Alternatively, I guess one could always squash the commits in conjunction with the merge.

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Feb 21, 2019

@oolongbrothers GitHub tracks force pushes on PRs now so we'll still be able to see a diff. Also, we usually use squash and merge when there are multiple commits. So either option should be fine.

@oolongbrothers oolongbrothers force-pushed the oolongbrothers:flatpak_remote-better-integration-tests branch from fa06e49 to 799bd6a Feb 23, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 23, 2019

@ansibot ansibot removed the needs_rebase label Feb 23, 2019

Excludes versions of Ubuntu older than 16.04 from tests
Since there are no flatpak packages available for these Ubuntu versions
@oolongbrothers

This comment has been minimized.

Copy link
Contributor Author

oolongbrothers commented Feb 23, 2019

Nice, tests are running now in Shippable.

I had to remove Ubuntu 14.04, since there are no flatpak packages available on versions older than 16.04.

@ansibot ansibot added the stale_ci label Mar 7, 2019

@ansibot ansibot removed the stale_ci label Mar 10, 2019

@oolongbrothers

This comment has been minimized.

Copy link
Contributor Author

oolongbrothers commented Mar 10, 2019

Anything else I should change or are we good to go?

@mattclay mattclay merged commit 8f354ea into ansible:devel Mar 20, 2019

1 check passed

Shippable Run 114831 status is SUCCESS.
Details
@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Mar 20, 2019

@oolongbrothers Thanks for the tests. One possible improvement for a future PR would be to convert the readme to a playbook to automate creation of the archive. That will make it easier for anyone who needs to make changes to the tests.

@oolongbrothers

This comment has been minimized.

Copy link
Contributor Author

oolongbrothers commented Mar 20, 2019

@mattclay Thanks for your feedback and for helping getting this merged.
Good point about converting the README to a playbook. I'll have a look at how easily I can get this done.
I am now working on getting the tests for the flatpak module in shape so that they can be run in CI as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.