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

Move io.go into io package #1782

Merged
merged 1 commit into from Nov 17, 2017
Merged

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Nov 17, 2017

As discussed in #1756

Move io*.go from the root package into an io package so the logic can be tested separate from integration tests.

If this looks good I can write a godoc string for the package and couple unit tests.

@dnephin dnephin force-pushed the move-io-pkg branch 2 times, most recently from bd297cd to bd46b0c Compare November 17, 2017 19:55
@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

Merging #1782 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1782   +/-   ##
=======================================
  Coverage   49.06%   49.06%           
=======================================
  Files          77       77           
  Lines        7719     7719           
=======================================
  Hits         3787     3787           
  Misses       3289     3289           
  Partials      643      643
Flag Coverage Δ
#linux 49.13% <ø> (ø) ⬆️
#windows 44.85% <ø> (ø) ⬆️

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 bc063f2...298dabc. Read the comment docs.

@crosbymichael
Copy link
Member

LGTM

io/io.go Outdated
@@ -1,4 +1,4 @@
package containerd
package io
Copy link
Member

Choose a reason for hiding this comment

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

This package name is problematic is that it overlaps with one of the more common packages, io, that is commonly used in conjunction with this package. The only better, minimal name I can come up with is cio, which would be short for "Container IO".

@stevvooe
Copy link
Member

LGTM after changing the name to cio.

task.go Outdated
@@ -5,7 +5,7 @@ import (
"context"
"encoding/json"
"fmt"
"io"
goio "io"
Copy link
Member

Choose a reason for hiding this comment

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

Unneeded now

container.go Outdated
// Spec returns the OCI runtime specification
Spec(context.Context) (*specs.Spec, error)
// Task returns the current task for the container
//
// If IOAttach options are passed the client will reattach to the IO for the running
// If io.Attach options are passed the client will reattach to the IO for the running
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: io -> cio?

Signed-off-by: Daniel Nephin <dnephin@gmail.com>
@dnephin
Copy link
Contributor Author

dnephin commented Nov 17, 2017

Thanks, fixed both of those issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants