Skip to content

Merging of subcommands #103

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
Merged
merged 1 commit into from
Mar 15, 2017
Merged

Merging of subcommands #103

merged 1 commit into from
Mar 15, 2017

Conversation

@zhouhao3
Copy link

@zhouhao3 zhouhao3 commented Jan 6, 2017 •
edited
Loading

This is some idea about #96

Fixes #96
Signed-off-by: zhouhao [email protected]

@zhouhao3 zhouhao3 force-pushed the main-test branch 4 times, most recently from 2c7d8d8 to fab1765 Compare January 6, 2017 05:53
@zhouhao3 zhouhao3 mentioned this pull request Jan 6, 2017
Closed
@vbatts
Copy link
Member

vbatts commented Feb 8, 2017

i think conceptually this SGTM

@xiekeyang
Copy link
Contributor

xiekeyang commented Mar 10, 2017

SGTM. need rebase.

@zhouhao3 zhouhao3 force-pushed the main-test branch 5 times, most recently from 90d1ef6 to 1f79983 Compare March 13, 2017 10:20
@zhouhao3
Copy link
Author

zhouhao3 commented Mar 13, 2017 •
edited
Loading

@vbatts @xiekeyang @coolljt0725 I think this change will be better. PTAL

@xiekeyang
Copy link
Contributor

xiekeyang commented Mar 13, 2017 •
edited
Loading

@q384566678 It seems that log-level is in progress on #46 . In that patch, @stevvooe suggest that we should not use log-level, instead to use debug mode. And you seems not replace fmt.Print by logrus with context. So I think you might had better to remove this stuff from this patch.

stevvooe
stevvooe reviewed Mar 13, 2017
View reviewed changes
man/oci-image-tool.1.md Outdated

# COMMANDS
**validate**
Validate one or more image files
Copy link
Contributor

@stevvooe stevvooe Mar 13, 2017

Choose a reason for hiding this comment

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

What is an "image file"?

stevvooe
stevvooe reviewed Mar 13, 2017
View reviewed changes
man/oci-image-tool.1.md Outdated
See **oci-image-tool-validate**(1) for full documentation on the **validate** command.

**unpack**
Unpack an image or image source layout
Copy link
Contributor

@stevvooe stevvooe Mar 13, 2017

Choose a reason for hiding this comment

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

What is an "image" or "image source layout"?

stevvooe
stevvooe reviewed Mar 13, 2017
View reviewed changes
man/oci-image-tool.1.md Outdated
Unpack an image or image source layout
See **oci-image-tool-unpack**(1) for full documentation on the **unpack** command.

**bundleCreate**
Copy link
Contributor

@stevvooe stevvooe Mar 13, 2017

Choose a reason for hiding this comment

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

Let's avoid camelCase in our command designs. This should employ <object> <verb> or use dashes bundle-create.

merging of subcommands
1423c5e 
Signed-off-by: zhouhao <[email protected]>
stevvooe
stevvooe reviewed Mar 15, 2017
View reviewed changes
cmd/oci-image-tool/unpack.go
var unpackCommand = cli.Command{
Name: "unpack",
Usage: "Unpack an image or image source layout",
Action: unpackHandle,
Copy link
Contributor

@stevvooe stevvooe Mar 15, 2017

Choose a reason for hiding this comment

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

unpackHandler.

stevvooe
stevvooe reviewed Mar 15, 2017
View reviewed changes
man/oci-image-tool.1.md
Validate the given file(s) against the OCI image specification
See **oci-image-tool-validate**(1) for full documentation on the **validate** command.

**unpack**
Copy link
Contributor

@stevvooe stevvooe Mar 15, 2017

Choose a reason for hiding this comment

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

This language still doesn't make the different between "unpack" and "create" clear.

@stevvooe
Copy link
Contributor

stevvooe commented Mar 15, 2017 •
edited by caniszczyk
Loading

LGTM

Please follow up on the issues with create / unpack.

Approved with PullApprove

@vbatts
Copy link
Member

vbatts commented Mar 15, 2017 •
edited by caniszczyk
Loading

LGTM

Approved with PullApprove

@vbatts vbatts merged commit b177811 into opencontainers:master Mar 15, 2017
@xiekeyang
Copy link
Contributor

xiekeyang commented Mar 16, 2017

@q384566678 @vbatts @stevvooe This PR call logrus function, without introduce it to vendor. It leads build fail on make tool:

$ make tool 
go build -o oci-image-tool ./cmd/oci-image-tool
cmd/oci-image-tool/main.go:21:2: cannot find package "github.com/Sirupsen/logrus" in any of:

At least we should import logrus to glide.yaml and then make update-deps, the package can be generated in vendor.
Please take a review, is it Right?

@xiekeyang xiekeyang mentioned this pull request Mar 16, 2017
Merged
@ddysher ddysher mentioned this pull request Mar 18, 2017
Merged
@zhouhao3 zhouhao3 deleted the main-test branch March 20, 2017 01:30
@zhouhao3 zhouhao3 mentioned this pull request Mar 24, 2017
Merged
@wking wking mentioned this pull request Apr 13, 2017
Closed
@xiekeyang xiekeyang mentioned this pull request Jun 29, 2017
Merged
@wking wking mentioned this pull request Jul 11, 2017
Open
This was referenced Jul 27, 2017
Open
Closed
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zhouhao3 @vbatts @xiekeyang @stevvooe