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 machine table to containers.conf #782

Merged
merged 1 commit into from Sep 24, 2021

Conversation

ashley-cui
Copy link
Member

Add machine teable to configure podman machine options. Move
machine_image to the machine table, and add disk size to the machine
table.

Signed-off-by: Ashley Cui acui@redhat.com

**disk_size**=10

The size of the disk in GB created when init-ing a podman-machine VM

Copy link
Member

Choose a reason for hiding this comment

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

should we go ahead and add cpus?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can wire that in now

Copy link
Member Author

Choose a reason for hiding this comment

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

And I guess memory as well?

@baude
Copy link
Member

baude commented Sep 23, 2021

@rhatdan ptal

Add machine teable to configure podman machine options. Move machine_image to the machine table, and add cups, disk size, and memory to the machine table.

Signed-off-by: Ashley Cui <acui@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Sep 23, 2021

LGTM
Nice job @ashley-cui

Comment on lines -284 to -285
// MachineImage is the image used when creating a podman-machine VM
MachineImage string `toml:"machine_image,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This field is not in podman 3.3, right? In this case I would want this PR and the podman wiring to get into podman 3.4 so that we do not have to worry about backwards compat for this field.

Copy link
Member

Choose a reason for hiding this comment

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

Yes this is not in 3.3

@rhatdan
Copy link
Member

rhatdan commented Sep 24, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 24, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui, rhatdan

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

@baude
Copy link
Member

baude commented Sep 24, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 24, 2021
@openshift-merge-robot openshift-merge-robot merged commit f49fe9b into containers:main Sep 24, 2021
@vrothberg
Copy link
Member

vrothberg commented Sep 27, 2021

This broke vendoring c/common in Podman. Please always make sure to open a PR against Podman and vendor in the commit once it has merged (e.g., go get github.com/containers/common@main after the PR merged).

Waiting for a release of c/common is a too big time window as it blocks others (in this case me) from working on c/common and Podman.

@Luap99
Copy link
Member

Luap99 commented Sep 27, 2021

@vrothberg There is containers/podman#11741?

@vrothberg
Copy link
Member

I took over Dan's PR to push things forward -> containers/podman#11753

@vrothberg There is containers/podman#11741?

CI is red. For breaking changes, c/common PRs should only get merged once the Podman PR is green. Then we can do the vendor dance and vendor main (e.g., containers/podman#11737).

@ashley-cui
Copy link
Member Author

Ahh okay my bad, will open the podman PR first next time

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.

None yet

6 participants