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

Add a WithPrivileged OCI constructor and the options needed to build it #2269

Merged
merged 2 commits into from Apr 4, 2018
Merged

Add a WithPrivileged OCI constructor and the options needed to build it #2269

merged 2 commits into from Apr 4, 2018
+235 −10

Conversation

justincormack
Copy link
Contributor

@justincormack justincormack commented Apr 3, 2018 •
edited

This is for my conversion of cri to use the containerd OCI functions.

I also added a Compose function because we all love functional programming...

Signed-off-by: Justin Cormack justin.cormack@docker.com

stevvooe
stevvooe reviewed Apr 3, 2018
View reviewed changes
oci/spec_opts.go Outdated
@@ -27,6 +27,17 @@ import (
// SpecOpts sets spec specific information to a newly generated OCI spec
type SpecOpts func(context.Context, Client, *containers.Container, *specs.Spec) error

func Compose(opts ...SpecOpts) SpecOpts {
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to add a godoc here.

@stevvooe
Copy link
Member

stevvooe commented Apr 3, 2018

LGTM

@codecov-io
Copy link

Codecov Report

Merging #2269 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2269      +/-   ##
==========================================
- Coverage    41.1%   41.06%   -0.04%     
==========================================
  Files          66       66              
  Lines        7768     7775       +7     
==========================================
  Hits         3193     3193              
- Misses       4070     4077       +7     
  Partials      505      505
Flag Coverage Δ
#windows 41.06% <0%> (-0.04%) ⬇️
Impacted Files Coverage Δ
oci/spec_opts.go 58.46% <0%> (-7.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1b3ea4...7a4f33c. Read the comment docs.

@Random-Liu
Copy link
Member

@justincormack This is neat! :) Thanks a lot

@AkihiroSuda
Copy link
Member

Can we have ctr flag for this?

@justincormack
Copy link
Contributor Author

@AkihiroSuda added.

@justincormack justincormack changed the title [WIP] Add a WithPrivileged OCI constructor and the options needed to build it Add a WithPrivileged OCI constructor and the options needed to build it Apr 4, 2018
@justincormack justincormack force-pushed the WithPrivileged branch 2 times, most recently from 21080b8 to 96a8c66 Compare April 4, 2018 12:02
@justincormack
Add a WithPrivileged OCI constructor and the options needed to build it
062c3a0 
Signed-off-by: Justin Cormack <justin.cormack@docker.com>
@justincormack
Add --privileged option to ctr run
9031910 
Signed-off-by: Justin Cormack <justin.cormack@docker.com>
@justincormack
Copy link
Contributor Author

Test added.

The appveyor failure looks unrelated...

I need to add some additional pieces around devices in privileged mode, and potentially there may be more pieces but I can do in follow up PRs.

mlaventure
mlaventure approved these changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@mlaventure mlaventure left a comment

Choose a reason for hiding this comment

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

LGTM

Restarted appveyor, but it is indeed unrelated.

@mlaventure
Copy link
Contributor

Yes, there seems to be an issue with platform manifest pulling:

--- FAIL: TestImagePullSomePlatforms (1.68s)
	client_test.go:297: expected a different number of pulled manifests

/cc @darrenstahlmsft @stevvooe

@dmcgowan
Copy link
Member

dmcgowan commented Apr 4, 2018 •
edited

The problem is with the test, I am fixing. The test is trying to do a multiplatform pull then checking how many manifests exists. One of the architectures it is trying to pull doesn't exist. It appears to pass on Linux because amd64 is already there so it is counting it, while on Windows it is not. There also appears to have been a change in the busybox image to remove 386, causing this failure to suddenly start.

@mlaventure mlaventure merged commit 9d9d1bc into containerd:master Apr 4, 2018
@justincormack justincormack deleted the WithPrivileged branch April 4, 2018 18:38
@mlaventure mlaventure added this to the 1.1 milestone Apr 4, 2018
@thaJeztah thaJeztah mentioned this pull request Sep 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevvooe stevvooe stevvooe left review comments

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@mlaventure mlaventure mlaventure approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@justincormack @stevvooe @codecov-io @Random-Liu @AkihiroSuda @mlaventure @dmcgowan