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

ci: also run integration test on Ubuntu #2492

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

bdrung
Copy link
Contributor

@bdrung bdrung commented Aug 19, 2023

Changes

Add a Dockerfile for Ubuntu (very similar to Debian) and run the integration tests also on Ubuntu.

I built the container locally and tested that the test cases from the integration test succeed there.

Checklist

  • I have tested it locally
  • I have reviewed and updated any documentation if relevant
  • I am providing new code and test(s) for it

Fixes #

@github-actions github-actions bot added test Issues related to testing github Issues related to .github labels Aug 19, 2023
@LaszloGombos
Copy link
Collaborator

LaszloGombos commented Aug 19, 2023

1./ The obvious concern with this PR is that it adds to the resource cost to the CI - specifically it runs 15 more tests for each CI run. My opinion as one of the maintainers is that a project has a lot to gain by having an active contributor from Debian/Ubuntu and if that active contributor is willing to invest in maintaining an Ubuntu CI then we should positively consider it.

2./ The somewhat less obvious concern with the extra test container is the potential churn and review/commit cost it takes to maintain it.

The Ubuntu container is almost an exact copy of the Debian container. Adding a new test container is a good time to discuss how we can we better maintain all of these test containers. There is a PR under review that is trying to address this concern - #2423

@bdrung - what do you think about the approach at #2423 ?

@LaszloGombos
Copy link
Collaborator

@bdrung Can you please also add this new container to manualtest

Copy link
Member

@aafeijoo-suse aafeijoo-suse left a comment

Choose a reason for hiding this comment

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

The Ubuntu container is almost an exact copy of the Debian container.

+1

Adding a new test container is a good time to discuss how we can we better maintain all of these test containers.

I'd love to see Ubuntu running on the CI, but adding a new container implies having to support it, we already have 5, and upstream resources (talking about people) are veeery limited...

Add a Dockerfile for Ubuntu (very similar to Debian) and run the
integration tests also on Ubuntu.

Signed-off-by: Benjamin Drung <[email protected]>
@bdrung
Copy link
Contributor Author

bdrung commented Aug 21, 2023

Added ubuntu to manialtest.

I'd love to see Ubuntu running on the CI, but adding a new container implies having to support it, we already have 5, and upstream resources (talking about people) are veeery limited...

Ubuntu will use dracut-install in initramfs-tools (see manual_add_modules is slow / consumes the most time). So at least dracut-install will become part of Ubuntu main (see [MIR] dracut) and part of the 1000 packages our Ubuntu Foundations team will maintain.

Copy link
Collaborator

@LaszloGombos LaszloGombos left a comment

Choose a reason for hiding this comment

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

I support adding Ubuntu to the CI.

upstream resources (talking about people) are veeery limited

The way I see it, landing this PR is actually the best way to solve the resource issue. We already got many fixes for all distributions (not just for Ubuntu) from @bdrung (and this is only week 2).

I think it is good to remember that each PR is reviewed independently and this PR does not necessary imply any Ubuntu specific changes (as that would be another PR, if any).

It is also good to remember that it is very easy to reverse this decision with a follow-up PR to remove the test container - just like we did in the past - 0b339d7 .

@aafeijoo-suse
Copy link
Member

Does this PR need to be merged before any of the Ubuntu tests go green?

@bdrung
Copy link
Contributor Author

bdrung commented Aug 21, 2023

Yes, the Ubuntu tests need the Ubuntu container, which is only built from the master branch.

@aafeijoo-suse aafeijoo-suse merged commit 8e9d89d into dracutdevs:master Aug 21, 2023
86 of 113 checks passed
@bdrung bdrung deleted the ubuntu-ci branch August 21, 2023 14:01
@LaszloGombos
Copy link
Collaborator

LaszloGombos commented Aug 21, 2023

#6 ERROR: failed to push ghcr.io/dracutdevs/ubuntu:latest: failed commit on ref "manifest-sha256:cf46c1d87c17a2c0d66b173690b7c9428e162f8fbe87f6071deaa81210167a90": unexpected status from PUT request to https://ghcr.io/v2/dracutdevs/ubuntu/manifests/sha256:cf46c1d87c17a2c0d66b173690b7c9428e162f8fbe87f6071deaa81210167a90: 403 Forbidden

..sadly.. investigating, but not very hopeful..

https://github.com/dracutdevs/dracut/pkgs/container/ubuntu

@LaszloGombos
Copy link
Collaborator

Update: this is now resolved.. here is a manual test run - https://github.com/dracutdevs/dracut/actions/runs/5927762437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github Issues related to .github test Issues related to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants