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.

Sign up for GitHub

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

Jump to bottom

Read kube_generate_type from containers.conf #18140

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Read kube_generate_type from containers.conf #18140

merged 1 commit into from
Apr 11, 2023

Conversation

umohnani8
Copy link
Member

Use the kube_generate_type from the containers.conf as the default value for the --type flag for kube generate. Override the default when userexplicitly sets the --type flag.

Does this PR introduce a user-facing change?

Use defaults from `kube_generate_type` in containers.conf for podman kube generate --type.

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 10, 2023
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Apr 10, 2023
edsantiago
edsantiago suggested changes Apr 10, 2023
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

One crucial bug, two minor ones (in system tests. I haven't looked at anything else)

test/system/710-kube.bats Outdated Show resolved Hide resolved
test/system/710-kube.bats Outdated Show resolved Hide resolved
test/system/710-kube.bats Outdated Show resolved Hide resolved
@umohnani8 umohnani8 force-pushed the deployments branch 2 times, most recently from e7c96b3 to 897132d Compare April 11, 2023 00:36
@umohnani8 umohnani8 requested a review from edsantiago April 11, 2023 01:05
edsantiago
edsantiago reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

System test LGTM now, thank you! Further review/approval is left as an exercise for others.

test/system/710-kube.bats Show resolved Hide resolved
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 11, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, umohnani8

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:
  • OWNERS [edsantiago,umohnani8]

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

@umohnani8
Copy link
Member Author

@rhatdan does testing containers.conf in test/e2e/containers_conf_test.go require a device to be mounted? Looks like the test I added is failing at the kube generation command because a linux device is being mounted but kube generate doesn't support that - https://api.cirrus-ci.com/v1/artifact/task/6482272522600448/html/int-podman-debian-12-root-host-boltdb.log.html#t--Set-kube_generate_type--1.

This is only happening in the containers_conf_test.go suite, generate_kube_test.go is fine.

vrothberg
vrothberg reviewed Apr 11, 2023
View reviewed changes
libpod/kube.go Outdated Show resolved Hide resolved
pkg/api/handlers/libpod/generate.go Outdated Show resolved Hide resolved
test/e2e/containers_conf_test.go Outdated Show resolved Hide resolved
@umohnani8
Read kube_generate_type from containers.conf
1bf5bdc 
Use the kube_generate_type from the containers.conf as
the default value for the --type flag for kube generate.
Override the default when userexplicitly sets the --type
flag.

Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
vrothberg
vrothberg reviewed Apr 11, 2023
View reviewed changes
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
@containers/podman-maintainers PTAL

@umohnani8
Copy link
Member Author

Since we have system tests, I dropped the e2e test for now as we were hitting a linux device being mounted issue that kube generate was unhappy with. Can't reproduce the issue locally though but will look into trying to add a e2e test in the follow up PR.

In the interest of time, I would like to get this in with just the system test.

@vrothberg
Copy link
Member

/lgtm
/hold

Given Ed dropped a LGTM already

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2023
@mheon
Copy link
Member

mheon commented Apr 11, 2023

LGTM
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 11, 2023
@openshift-merge-robot openshift-merge-robot merged commit cf3374e into containers:main Apr 11, 2023
This was referenced Apr 11, 2023
Open
Closed
@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 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vrothberg vrothberg vrothberg left review comments

@edsantiago edsantiago edsantiago approved these changes

Assignees

@vrothberg vrothberg

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@umohnani8 @vrothberg @mheon @edsantiago @openshift-merge-robot