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 unit test for ValidateLayout function #310

Closed
wants to merge 2 commits into from
Closed

add unit test for ValidateLayout function #310

wants to merge 2 commits into from

Conversation

xiekeyang
Copy link
Contributor

Signed-off-by: xiekeyang xiekeyang@huawei.com

@xiekeyang
Copy link
Contributor Author

cc @wking @stevvooe @jonboulle @runcom

"config": {
"AttachStderr": false,
"AttachStdin": false,
"AttachStdout": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see anything about these in serialization.md. Can we remove them?

Copy link
Contributor Author

@xiekeyang xiekeyang Sep 14, 2016

Choose a reason for hiding this comment

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

removed, I adapt spec patten as serialization.md#image-json-description

@xiekeyang
Copy link
Contributor Author

@wking
fixed, PTAL.
cc @runcom @jonboulle @stevvooe

xiekeyang added 2 commits September 14, 2016 18:14
It is reused on many place, so should be built a function.

Signed-off-by: xiekeyang <xiekeyang@huawei.com>
Signed-off-by: xiekeyang <xiekeyang@huawei.com>
@vbatts
Copy link
Member

vbatts commented Sep 15, 2016

LGTM

Approved with PullApprove

@runcom
Copy link
Member

runcom commented Sep 15, 2016

are we merging this here? or does this need to be moved (and possibly merged) in image-tools?

@wking
Copy link
Contributor

wking commented Sep 15, 2016

On Thu, Sep 15, 2016 at 07:07:52AM -0700, Antonio Murdaca wrote:

are we merging this here? or does this need to be moved (and
possibly merged) in image-tools?

It can be merged on the command line, just add an image-spec remote.
Or we can move it over if folks want a green button ;).

@runcom
Copy link
Member

runcom commented Sep 15, 2016

It can be merged on the command line, just add an image-spec remote.

and that's is awful to me given we have another repo and the code here is just for image-tools, do we want to cherry-pick/merge every outstanding PR from image-spec once merged here - anyway whatever

@wking
Copy link
Contributor

wking commented Sep 15, 2016

On Thu, Sep 15, 2016 at 07:24:40AM -0700, Antonio Murdaca wrote:

It can be merged on the command line, just add an image-spec remote.

and that's is awful to me given we have another repo and the code
here is just for image-tools, do we want to cherry-pick/merge every
outstanding PR from image-spec once merged here - anyway whatever

You have to migrate them over somehow. image-tools has preserved the
full image-spec history, so there's no need to cherry-pick or rebase
(unless subsequent image-tools development leads to a conflict you
need to rebase around). Merging from the command line seems easier
than asking OPs to re-open new PRs, assuming you've already collected
the LGTMs (and the maintainer sets are currently the same, so
PullApprove will flag those appropriately).

@vbatts
Copy link
Member

vbatts commented Sep 15, 2016

Are these sources just for image-tools though?

@runcom
Copy link
Member

runcom commented Sep 15, 2016

Are these sources just for image-tools though?

they're seem to be just used in cmd/ which has moved to image-tools (according to my ability with grep which may be bad - that's basically the reason I left image/ in image-tools)

@wking
Copy link
Contributor

wking commented Sep 15, 2016

On Thu, Sep 15, 2016 at 07:35:12AM -0700, Antonio Murdaca wrote:

Are these sources just for image-tools though?

they're seem to be just used in cmd/ which has moved to
image-tools (according to my ability with grep which may be bad -
that's basically the reason I left image/ in image-tools)

And I expect image-tools to eventually expose this sort of thing in Go
libraries like runtime-tools has been doing (starting with
opencontainers/runtime-tools#130).

@vbatts
Copy link
Member

vbatts commented Sep 15, 2016

i guess i meant, like runtime-spec has ./specs-go/ that are used by the runtime-tools and runc, I figured there would be a similar relationship here. I could likely be wrong though.

@runcom
Copy link
Member

runcom commented Sep 15, 2016

i guess i meant, like runtime-spec has ./specs-go/ that are used by the runtime-tools and runc, I figured there would be a similar relationship here. I could likely be wrong though.

@vbatts I left ./specs-go/ which contains the types here in image-spec indeed. image/ is just a set of functions used only in image-tools

@vbatts
Copy link
Member

vbatts commented Sep 15, 2016

ah. my bad. you're right.

On Thu, Sep 15, 2016 at 10:47 AM, Antonio Murdaca notifications@github.com
wrote:

i guess i meant, like runtime-spec has ./specs-go/ that are used by the
runtime-tools and runc, I figured there would be a similar relationship
here. I could likely be wrong though.

@vbatts https://github.com/vbatts I left ./specs-go/ which contains the
types here in image-spec indeed. image/ is just a set of functions used
only in image-tools


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#310 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEF6bRxzKDJ-R2GaFIoDMB2yJyma2Upks5qqVqHgaJpZM4J8dfx
.

@wking
Copy link
Contributor

wking commented Sep 15, 2016

On Thu, Sep 15, 2016 at 07:47:35AM -0700, Antonio Murdaca wrote:

i guess i meant, like runtime-spec has ./specs-go/ that are used
by the runtime-tools and runc, I figured there would be a similar
relationship here. I could likely be wrong though.

@vbatts I left ./specs-go/ which contains the types here in
image-spec indeed. image/ is just a set of functions used only in
image-tools

Yeah. +1 for leaving dead stuff (JSON Schema, Go types) in image-spec
but putting the moving parts (image/, cmd/) in image-tools.

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.

5 participants