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

independently unpacking layer functions #11

Closed
wants to merge 2 commits into from
Closed

independently unpacking layer functions #11

wants to merge 2 commits into from

Conversation

xiekeyang
Copy link
Contributor

cc @wking @runcom @stevvooe

At early discussion, @wking and I doubt that manifest.unpack() is not very close to manifest structure.
Actually, manifest.unpack() is only responsible for unpacking layers, which list can be gotten from manifest. It can be done without a real manifest. It is odd to use it as a method of manifest structure.
So, I place 2 layer functions to layer.go file. and place their unit test to layer_test.go.
From my side it looks more reasonable and go easily forward to reconstructing. And I'd like to get your proposal.

Signed-off-by: xiekeyang [email protected]

@wking
Copy link
Contributor

wking commented Sep 19, 2016

On Sun, Sep 18, 2016 at 08:04:04PM -0700, xiekeyang wrote:

Actually, manifest.unpack() is only responsible for unpacking
layers, which list can be gotten from manifest. It can be done
without a real manifest. It is odd to use it as a method of manifest
structure. So, I place 2 layer functions to layer.go file. and
place their unit test to layer_test.go.

This makes sense to me. However, if the in-flight #8 lands pulling in
an external library for manipulating layers it may be a moot point.
We should probably idle this PR until the maintainers and other
interested parties have had time to weigh in on #8.

@xiekeyang
Copy link
Contributor Author

PTAL.

@glestaris
Copy link
Contributor

Also #3 is refactoring the unpacking area slightly. One of us may have to rebase :)

@xiekeyang
Copy link
Contributor Author

@glestaris Yes. According to previous discussion, the manifest_test.go is inproper because it is just for layer unpacking. So this PR should stand firstly, and it need more refactor following to #4 .
I read #3 and think that rebase is necessary but not very difficult, which I can help to do.
Thanks.

@vbatts
Copy link
Member

vbatts commented Oct 6, 2016

please rebase

@xiekeyang
Copy link
Contributor Author

rebased. PTAL
cc @wking @vbatts

@xiekeyang
Copy link
Contributor Author

@vbatts It has been rebased.

@xiekeyang
Copy link
Contributor Author

@opencontainers/image-tools-maintainers
I think this PR is ready to be merged. PTAL.
The main purpose is to set up layer.go and unit test file, to collect layer unpacking functions, which mix with manifest. We are and will handle layer. It had better to involve them in individual files.

@wking
Copy link
Contributor

wking commented Nov 18, 2016

On Thu, Nov 17, 2016 at 07:41:48PM -0800, xiekeyang wrote:

I think this PR is ready to be merged. PTAL.

I still 1 think this depends on how we intend to handle #8. As
@glestaris pointed out 2, using a vendored/forked library only for
creation but not for unpacking is strangely asymmetric. Do we have a
consensus on where #8 is headed? Is it likely to land? Is it's new
utils/ package (forked from github.com/containers/storage, which is
forked from docker/docker) expected to grow/fork unpacking logic?

@xiekeyang
Copy link
Contributor Author

As evolution on image-spec, this PR should be closed, and waiting for a solid design framework of CAS.
@coolljt0725 please review #8
@wking please review #5
Consider how to handle them.

@xiekeyang xiekeyang closed this Feb 16, 2017
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.

4 participants