-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[3.4] Allow machine options to be set from containers.conf #11742
Conversation
@@ -247,8 +247,8 @@ github.com/containernetworking/plugins v0.9.1/go.mod h1:xP/idU2ldlzN6m4p5LmGiwRD | |||
github.com/containers/buildah v1.23.0 h1:qGIeSNOczUHzvnaaOS29HSMiYAjw6JgIXYksAyvqnLs= | |||
github.com/containers/buildah v1.23.0/go.mod h1:K0iMKgy/MffkkgELBXhSXwTy2HTT6hM0X8qruDR1FwU= | |||
github.com/containers/common v0.44.0/go.mod h1:7sdP4vmI5Bm6FPFxb3lvAh1Iktb6tiO1MzjUzhxdoGo= | |||
github.com/containers/common v0.44.1 h1:B4cSH5ZJS69Kkum0hFwvX3TL9XSmEXj5b+f5okFzAVg= | |||
github.com/containers/common v0.44.1/go.mod h1:7sdP4vmI5Bm6FPFxb3lvAh1Iktb6tiO1MzjUzhxdoGo= | |||
github.com/containers/common v0.46.0 h1:95zB7kYBQJW+aK5xxZnaobCwoPyYOf85Y0yUx0E5aRg= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a problem - I think we've already vendor-danced and settled on 0.44 - @vrothberg @TomSweeneyRedHat Can you folks confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, @mheon!
I don't think we did the vendor dance yet. If we did, then we must stick v0.44.X. Otherwise, we still have time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomSweeneyRedHat are you human, or are you dancer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the new c/common for the new machine fields, otherwise we would have a breaking change in the next version because the Engine.MachineImage
field no longer exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see #11745, arch users already have the new containers.conf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ashley-cui if you were to backport containers/common#782 into the c/common v0.44 branch on c/common and then we created a v.0.44.2 release, would that work for you or do you have other dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets talk about this one today ... i'm not so sure im comfy with either option.
51f1f4f
to
58f0fd0
Compare
code LGTM |
Signed-off-by: Ashley Cui <[email protected]>
CPUS, memory, disk size, and image path defaults can be set from [machine] table in containers.conf [NO TESTS NEEDED] Signed-off-by: Ashley Cui <[email protected]>
58f0fd0
to
0c2183d
Compare
/lgtm |
/approve |
[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 |
Closing in favor of vendoring common 0.44 |
Allow machine options to be set from containers.conf
CPUS, memory, disk size, and image path defaults can be set from [machine] table in containers.conf
Bump containers/common to v0.46.0
[NO TESTS NEEDED]