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

play.go: remove volumes with kube down --force #18814

Merged

Conversation

danishprakash
Copy link
Contributor

Closes #18797

kube down: remove volumes when --force is specified

Signed-off-by: danishprakash [email protected]

pkg/domain/infra/abi/play.go Outdated Show resolved Hide resolved
test/e2e/play_kube_test.go Outdated Show resolved Hide resolved
@baude
Copy link
Member

baude commented Jun 7, 2023

/approve
LGTM

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2023
@danishprakash danishprakash force-pushed the kube-down-volrm branch 2 times, most recently from d24e111 to 5709755 Compare June 12, 2023 12:56
@TomSweeneyRedHat
Copy link
Member

All kinds of red tests @danishprakash

@danishprakash danishprakash force-pushed the kube-down-volrm branch 7 times, most recently from 3e1a5f7 to 99ad806 Compare June 14, 2023 12:08
@danishprakash
Copy link
Contributor Author

All kinds of red tests @danishprakash

Yeah, I was trying to work around the fact that the volumes created by podman are named differently than what's specified in the k8s manifest. The current solution is in alignment with how we create volumes out of the manifest.

But in the longer run, I believe KubeVolume could have a Name attribute which would be the direct equivalent of the volume name in the k8s manifest. That is of course, I'm missing out on a specific design decision that involved using Source as the volume name for all kinds of volumes (the current implementation).

@rhatdan
Copy link
Member

rhatdan commented Jun 16, 2023

Should we change the naming of kubernetes volumes to match kubernetes?

@danishprakash
Copy link
Contributor Author

I would say yes, should that come under the scope of this PR or another? I think for the issue this was opened, this change is sufficient. PTAL.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

One nit on the test, other than that LGTM

@Luap99 PTAL

test/e2e/play_kube_test.go Show resolved Hide resolved
@danishprakash danishprakash force-pushed the kube-down-volrm branch 3 times, most recently from a88e761 to 23050a5 Compare June 19, 2023 13:45
* add e2e test

Signed-off-by: danishprakash <[email protected]>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
@Luap99 PTAL

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, danishprakash, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 416b4ee into containers:main Jun 28, 2023
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman kube down --force doesn't seem to work
7 participants