Adding support for podman play volumes #8266
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aidan-plenert-macdonald The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/domain/infra/abi/play_test.go
Outdated
| @@ -252,3 +258,86 @@ func TestEnvVarValue(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func TestContainerEngine_PlayKube(t *testing.T) { | |||
There was a problem hiding this comment.
This test doesn't work. I need to mock out the runtime somehow.
|
Thanks @aidan-plenert-macdonald |
Signed-off-by: Aidan Macdonald <[email protected]>
|
We generally avoid unit tests due to the complexities of mocking the increasingly-large Runtime; instead, we use extensive E2E tests of the functionality in question using a full Podman executable. |
|
Okay. That's fair. This might answer my question in another issue I opened.
I was offering to rework some of the code to make it easier to unit test.
If you don't want that, I'll just do e2e tests. They are just slower, so I
typically do unit tests first.
…On Sat, Nov 7, 2020, 12:26 Matthew Heon ***@***.***> wrote:
We generally avoid unit tests due to the complexities of mocking the
increasingly-large Runtime; instead, we use extensive E2E tests of the
functionality in question using a full Podman executable.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8266 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABTBOJZGL6KEHVRHKPPED4LSOWUO7ANCNFSM4TNOHPVQ>
.
|
|
@aidan-plenert-macdonald We would love to have more unit tests, but some are just too difficult to setup, so we fall back to E2E. |
|
This is a great start to #5788 and I look forward to being able to make volumes with k8s config. This is only part of what's needed for that issue though. This PR allows creating volumes, but it doesn't allow mounting them into a container. We should also add support for volumes to I think it makes sense to keep the scope of this issue as it is, but also keep track of these other features that still need to be added. @aidan-plenert-macdonald, the smoke tests are failing due to a couple git commit issues. Could you please squash your commits into a single commit, make sure the squashed commit still has your DCO signoff, and make sure the length of the commit title is 90 characters or less? For the commit title, I would suggest "Add support for podman play volumes". See https://chris.beams.io/posts/git-commit/ Thanks for working on this and I look forward to seeing this merged! |
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Should have marked a draft. I'm still trying to get the e2e tests to pass locally, then I'll do the |
openshift-ci-robot
added
the
needs-rebase
|
@aidan-plenert-macdonald: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@aidan-plenert-macdonald, are you still working on it? |
|
I am currently working on #9129 and both issues change the same files. So it might be nice to have this one first. If there is no activity on this one, I can continue with the work / changes that are pending. |
|
@EduardoVega Take this over if you can. |
|
I'll do. Thanks |
|
A friendly reminder that this PR had no activity for 30 days. |
|
@EduardoVega Good to close, given #9935 has merged? |
|
@mheon correct. |
github-actions
bot
added
the
locked - please file new issue/PR
To resolve #5788