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

About the merging of subcommands #96

Closed
zhouhao3 opened this issue Dec 6, 2016 · 24 comments
Closed

About the merging of subcommands #96

zhouhao3 opened this issue Dec 6, 2016 · 24 comments

Comments

@zhouhao3
Copy link

zhouhao3 commented Dec 6, 2016

I do not understand why the image-tools in the three sub-command split into three software, I think these three commands should be combined into a tool, after all, they belong to image-tools, I think the merger will Easy to understand and application, if I was a user, I would think that the use of a software implementation of the three commands than the use of three software to implement a command to be more convenient.
I currently have a plan to merge, don't know how you think .

@zhouhao3
Copy link
Author

zhouhao3 commented Dec 7, 2016

@wking
Copy link
Contributor

wking commented Dec 7, 2016

I don't care either way.

@coolljt0725
Copy link
Member

I prefer the merging just like oci-runtime-tool, but other maintainers may have other opinions :)

@stevvooe
Copy link
Contributor

stevvooe commented Jan 6, 2017

Could you please provide an example of the proposal?

I'd like to see some actual design work.

@zhouhao3
Copy link
Author

zhouhao3 commented Jan 6, 2017

@stevvooe #103 is some of my ideas, if possible, I will continue to improve.

@stevvooe
Copy link
Contributor

stevvooe commented Jan 6, 2017

@q384566678 That is a PR.

Do you have a design document?

@zhouhao3
Copy link
Author

@stevvooe This picture is my overall idea, the above separation of the three functions into a whole function. This is not only convenient to use, but also to reduce the duplication of some functions.

image-tool

@stevvooe
Copy link
Contributor

@q384566678 Ok, that is a start.

However, it looks like this is just thrown together. Let's use an object verb pattern to structure the commands:

oci-image-tool [object] [verb]
oci-image-tool image validate IMAGE
oci-image-tool image unpack IMAGE
oci-image-tool bundle create DST IMAGE

The above is just an example, but as you can see, we need to do some thinking about the objects that exist and the actions that can be taken with them.

@zhouhao3
Copy link
Author

@stevvooe OK, I will rectify the next, thank you for your suggestion.

@stevvooe
Copy link
Contributor

@q384566678 Please don't make changes on this PR.

Make a design document with a proposal.

@zhouhao3
Copy link
Author

@stevvooe I don't quite understand what you said design document, design ideas or the final generated file? would you please say this to me?

@stevvooe
Copy link
Contributor

@q384566678 Write something that describes what you plan to do. It should include details about what commands and subcommands there are.

@zhouhao3
Copy link
Author

zhouhao3 commented Jan 12, 2017

@stevvooe Here's what I want to do:

oci-image-tool [global options] command [command options] [arguments...]

commands:
validate
bundleCreate
unpack
(In each order will have their own sub-command below)

global options:
--version
--help
(I'm going to add other options later in global options)

@stevvooe
Copy link
Contributor

@q384566678 Let's try and organize around object verb. For example, your proposal begs the question "What am I unpacking?" and "What am I validating?". Can we achieve the same with the following:

bundle validate
bundle create
bundle unpack

However, I am still not sure what a bundle and what the difference between unpacking and creating is.

I think opencontainers/image-spec#467 needs to be fleshed out further.

@zhouhao3
Copy link
Author

zhouhao3 commented Feb 3, 2017

@stevvooe I think the object here is the input file, verb is the next command to be executed. So I do not think it needs to be so complicated.

@stevvooe
Copy link
Contributor

stevvooe commented Feb 3, 2017

@q384566678 Its not about making it complicated. Its about making it reflect the actual problem. I am pretty familiar with this area and when I look at the current state of the image tool and the proposed state, I have no clue what it is trying to do.

I would really love for someone to pick up opencontainers/image-spec#467 and start doing the engineering work of defining what these actions are. Without it, we are just shooting in the dark and hope stuff sticks to the wall.

@xiekeyang
Copy link
Contributor

I think we might can submit a PR firstly, to combine the sub-packages under cmd directory, to present only one main file. This can build a basic framework, and not to conflict next engineering work of really actions. We've been blocked a little long time.

@zhouhao3
Copy link
Author

I have already submitted #103.

@xiekeyang
Copy link
Contributor

I have already submitted #103.

Thanks, I see and am reading it.

@stevvooe
Copy link
Contributor

@xiekeyang @q384566678 As requested earlier, it would be best if we could see some up front design work on the command before going forward. Please see opencontainers/image-spec#588 (comment) for an example. That comment shows a command and its output. I think we really need to see this kind of detail tied to use cases before we can move forward with anything.

Also, after taking a look at #103, it looks like it is introducing concepts that don't exist. For example, it talks about an "image file" and "image source layout". Neither of these are really defined by the specification. Without rigor here, we may end up introducing a component that should be specified but is not.

For the most part, the image-tools should focus on packing and unpacking to a bundle and config. I think we can break it down into these use cases:

  1. Validate an oci "image layout" archive file.
  2. Unpack an oci "image layout" into a bundle directory with a config.
  3. Pack an oci "image layout" file from a bundle directory.

If we drive the design from these uses (and don't get distracted), I think we can ensure the tool will be much more aligned. There are probably other cases we need to consider, but let's focus on making these rock solid before adding functionality.

Let me know if more clarification is needed.

@xiekeyang
Copy link
Contributor

xiekeyang commented Mar 15, 2017

@stevvooe

I totally agree with you not to add any functionality in this patch.

I had suggested to just combine current tools as format oci-image <subcommand> [options]. That bring benefits as unifying manual, unifying public command options, remove redundant code... Hopefully this patch can help to prepare a better command format (only this).

To be honest I'm a little distraught when developing logger feature, because I have to duplicate such as help words, cmd flags, initialization... I worry other developers will encounter same problem.

Do you think it is worthy to move this PR forward to what I suggest above, or we should end up this?

@xiekeyang
Copy link
Contributor

@stevvooe sorry for confusing words. Do you think it is worthy to move #103 forward to what I suggest above, or we should end up it?

@stevvooe
Copy link
Contributor

@stevvooe sorry for confusing words. Do you think it is worthy to move #103 forward to what I suggest above, or we should end up it?

@xiekeyang #103 still doesn't make the difference between unpack and create clear. If the goal of #103 is to merge the commands, we can go through it, but we need to see the engineering work happen. I keep seeing PRs coming by but none of them seem to address the base concerns that I've attempted to spell out above.

@stevvooe
Copy link
Contributor

@xiekeyang Perhaps, we go ahead and merge #103 and then follow up the problems with unpack and create. Let's just not let those concerns drop on the floor.

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

No branches or pull requests

5 participants