Skip to content

OCI Modifiers for Windows#2981

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
lowenna:jjh/ocioptions
Feb 6, 2019
Merged

OCI Modifiers for Windows#2981
crosbymichael merged 1 commit intocontainerd:masterfrom
lowenna:jjh/ocioptions

Conversation

@lowenna
Copy link
Copy Markdown

@lowenna lowenna commented Feb 5, 2019 •
edited
Loading

Signed-off-by: John Howard [email protected]

No longer sets defaults for

  • .Process.ConsoleSize
  • .Windows.IgnoreFlushesDuringBoot
  • .Windows.Network.AllowUnqualifiedDNSQuery

Adds helper functions and tests for

  • WithWindowsIgnoreFlushesDuringBoot
  • WithWindowNetworksAllowUnqualifiedDNSQuery

Updates ctr run on Windows to use the new helper functions,
ConsoleSize is already handled.

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Feb 5, 2019

@jterry75 @crosbymichael @dmcgowan PTAL. I'm sure there will be more to add, but this is the immediate first one where things are being defaulted in the default Windows spec which shouldn't be.

@lowenna lowenna requested a review from jterry75 February 5, 2019 20:57
@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 5, 2019 •
edited
Loading

Codecov Report

Merging #2981 into master will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2981      +/-   ##
==========================================
- Coverage   43.98%   43.96%   -0.02%     
==========================================
  Files         102      102              
  Lines       10873    10881       +8     
==========================================
+ Hits         4782     4784       +2     
- Misses       5358     5362       +4     
- Partials      733      735       +2
Flag Coverage Δ
#linux 47.58% <100%> (-0.06%) ⬇️
#windows 41.18% <66.66%> (-0.02%) ⬇️
Impacted Files Coverage Δ
oci/spec.go 93.4% <100%> (-0.32%) ⬇️
oci/spec_opts_windows.go 77.77% <64.7%> (-22.23%) ⬇️

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 b94b99d...59ea134. Read the comment docs.

jterry75
jterry75 reviewed Feb 5, 2019
View reviewed changes
Comment thread cmd/ctr/commands/run/run_windows.go Outdated
jterry75
jterry75 reviewed Feb 5, 2019
View reviewed changes
Comment thread cmd/ctr/commands/run/run_windows.go Outdated
jterry75
jterry75 reviewed Feb 5, 2019
View reviewed changes
Comment thread oci/spec_opts_windows_test.go Outdated
jterry75
jterry75 reviewed Feb 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

Questions

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Feb 6, 2019

@jterry75 Updated

OCI Modifiers for Windows
59ea134 
Signed-off-by: John Howard <[email protected]>

Needed for the containerd work on Windows and integrating the
oci package from containerd into moby.

No longer sets defaults for
 - .Process.ConsoleSize
 - .Windows.IgnoreFlushesDuringBoot
 - .Windows.Network.AllowUnqualifiedDNSQuery

Adds helper functions and tests for
 - WithWindowsIgnoreFlushesDuringBoot
 - WithWindowNetworksAllowUnqualifiedDNSQuery

Updates `ctr run` on Windows to use the new helper functions,
ConsoleSize is already handled.
jterry75
jterry75 approved these changes Feb 6, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jterry75 jterry75 left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 6, 2019

@jterry75 do we have a plan for updating CRI now that we changed the defaults?

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 6, 2019

LGTM

@crosbymichael crosbymichael merged commit 0b89d42 into containerd:master Feb 6, 2019
@ehotinger
Copy link
Copy Markdown
Contributor

ehotinger commented Feb 7, 2019

@jhowardmsft what's with the oci.test.exe binary here?

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 7, 2019

@ehotinger nice catch, i didn't see that

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 7, 2019

You want to submit a PR to remove it?

@ehotinger ehotinger mentioned this pull request Feb 7, 2019
Merged
@ehotinger
Copy link
Copy Markdown
Contributor

ehotinger commented Feb 7, 2019

Done, will abandon if it serves a purpose, just noticed it after updating remotes by chance and didn't see an explanation from this PR.

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Feb 7, 2019

You want to submit a PR to remove it?

Ugh. Yes, will submit a PR very shortly.

@lowenna
Copy link
Copy Markdown
Author

lowenna commented Feb 7, 2019

Oh, already done. Thanks @ehotinger

@lowenna lowenna deleted the jjh/ocioptions branch February 7, 2019 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jterry75 jterry75 jterry75 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@lowenna @codecov-io @crosbymichael @ehotinger @jterry75