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

Add ContainerPodman class #130

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Add ContainerPodman class #130

merged 1 commit into from
Sep 20, 2024

Conversation

bastelfreak
Copy link
Member

@bastelfreak bastelfreak commented Mar 17, 2024

This allows us to set the beaker hypervisor not only to docker, but also to docker_podman. This enables us to implement podman specific patches (if we ever need to). But it also makes it easier for modules to specify if they want to use podman or docker. See voxpupuli/gha-puppet#48 for reference.

Copy link

codecov bot commented Mar 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.37%. Comparing base (1f99adf) to head (7dab835).
Report is 23 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #130   +/-   ##
=======================================
  Coverage   86.37%   86.37%           
=======================================
  Files           1        1           
  Lines         279      279           
=======================================
  Hits          241      241           
  Misses         38       38           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ekohl
Copy link
Member

ekohl commented Mar 17, 2024

Given there's no difference, I wonder if this is a benefit. Would it make sense to go the other direction and use container? So if you need to specialize it becomes container_podman, container_docker and container_swarm.

@bastelfreak
Copy link
Member Author

I've no opinions here. I'm fine with every implementation.

@evgeni
Copy link
Member

evgeni commented Mar 27, 2024

FWIW, I also think that this being beaker-container with docker, podman and swarm being sub-classes of Beaker::Container would be cleaner. That said, I hate renaming gems, so…

@evgeni
Copy link
Member

evgeni commented Mar 27, 2024

Or, we keep the gem beaker-docker, and the "main" implementation (for now) docker, but introduce container and container_<flavour>, all effectively aliased to docker for now as that's where the code lives.

@bastelfreak
Copy link
Member Author

@evgeni like this? And then use container_podman as hypervisor in GHA-puppet and not docker_podman?

require 'beaker/hypervisor/docker'

module Beaker
class ContainerPodman < Beaker::Docker
Copy link
Member Author

Choose a reason for hiding this comment

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

mhm or should it inherit from Beaker::Container?

Copy link
Member

Choose a reason for hiding this comment

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

Technically, Beaker::Container, not that it makes a difference today…

@evgeni
Copy link
Member

evgeni commented Mar 27, 2024

@evgeni like this? And then use container_podman as hypervisor in GHA-puppet and not docker_podman?

Yepp

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this is a good start, but I'd lean to:

  • Rename Beaker::Docker to Beaker::Container
  • Introduce Beaker::ContainerDocker, Beaker::ContainerPodman and Beaker::ContainerSwam that inherit from Beaker::Container

Then deprecate Beaker::Docker. At first we can keep all the code in a single class but later we can differentiate in different ways.

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

New GH mobile app stinks

lib/beaker/hypervisor/container_podman.rb Outdated Show resolved Hide resolved
This allows us to set the beaker hypervisor not only to `docker`,
but also to `docker_podmand`. This enables us to implement podman
specific patches (if we ever need to). But it also makes it easier
for modules to specify if they want to use podman or docker. See
voxpupuli/gha-puppet#48 for reference.
@evgeni evgeni changed the title Add DockerPodman class Add ContainerPodman class Sep 20, 2024
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

One day I'll figure out why these are called Beaker::Something but live in beaker/hypervisor/something. Today is not the day to think about that tho.

@evgeni evgeni merged commit 76a2d38 into voxpupuli:master Sep 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants