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

Conversation

oolongbrothers
Copy link
Contributor

@oolongbrothers 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
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

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. labels Feb 20, 2019
@gundalow
Copy link
Contributor

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

What is the total size of the dummy repo?

@gundalow
Copy link
Contributor

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

@gundalow gundalow added test This PR relates to tests. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Feb 21, 2019
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 21, 2019
@ansibot ansibot removed the core_review In order to be merged, this PR must follow the core review workflow. label Feb 21, 2019
@oolongbrothers
Copy link
Contributor Author

@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
Copy link
Member

@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.

@gundalow gundalow added support:community This issue/PR relates to code supported by the Ansible community. module This issue/PR relates to a module. and removed needs_triage Needs a first human triage before being processed. labels Feb 22, 2019
@oolongbrothers oolongbrothers force-pushed the flatpak_remote-better-integration-tests branch from fa06e49 to 799bd6a Compare February 23, 2019 13:30
@ansibot
Copy link
Contributor

ansibot commented Feb 23, 2019

@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Feb 23, 2019
Since there are no flatpak packages available for these Ubuntu versions
@oolongbrothers
Copy link
Contributor Author

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 This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 7, 2019
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 10, 2019
@oolongbrothers
Copy link
Contributor Author

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

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Mar 18, 2019
@mattclay mattclay merged commit 8f354ea into ansible:devel Mar 20, 2019
@mattclay
Copy link
Member

@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
Copy link
Contributor Author

@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.

@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants