Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

docs: update the sandbox operation API table #728

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

YchauWang
Copy link

Sync the api from the runtime codes to the documentation. Remove
sandbox.Pause() and sandbox.Resume() apis and add some missing methods
in this table.

Fixes: #727

Signed-off-by: Ychau Wang wangyongchao.bj@inspur.com

@YchauWang YchauWang requested a review from a team as a code owner September 8, 2020 02:11
Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Thanks @YchauWang. Not strictly required, but you could add a second commit that sorts this table by Name.

lgtm

Sync the api from the runtime codes to the documentation. Remove
sandbox.Pause() and sandbox.Resume() apis and add some missing methods
in this table. And sort this table by Feature and Name.

Fixes: kata-containers#727

Signed-off-by: Ychau Wang <wangyongchao.bj@inspur.com>
Copy link
Member

@c3d c3d left a comment

Choose a reason for hiding this comment

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

lgtm with a minor capitalization fix

|`sandbox.Release()`| Release a sandbox data structure, close connections to the agent, and quit any goroutines associated with the sandbox. Mostly used for daemon restart.|
|`sandbox.Start()`| Start a a sandbox and the containers making the sandbox.|
|`sandbox.Status()`| Get the status of the sandbox and containers.|
|`sandbox.Stop()`| Stop a sandbox and Destroy the containers in the sandbox..|
Copy link
Member

Choose a reason for hiding this comment

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

Nit: lowercase "destroy" (otherwise suggests it's a method name)

Copy link
Member

Choose a reason for hiding this comment

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

In doing reviews like this, you realize how stupid the Go convention of capitalizing public methods really is. It's really pointlessly painful to find the exports in a file from an editor. Even guru does not help. Any good suggestion there?

Copy link
Member

Choose a reason for hiding this comment

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

Added #729 about missing documentation for types.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @c3d, I don't have any means about the problem of Go. Thanks Christophe, the types are important for the api docs. I will fix the #729 issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@c3d - I tend to either use a crazy grep regex, but also have a golang program that uses ast + parse packages to list functions (it "works" but doesn't list the receiver for methods yet 😢)

@amshinde amshinde merged commit a5574b1 into kata-containers:master Sep 9, 2020
@amshinde amshinde added the needs-forward-port Changes need to be applied to a newer branch / repository label Sep 9, 2020
@amshinde
Copy link
Member

amshinde commented Sep 9, 2020

@YchauWang Can you port this change to the Kata 2.0 repository as well : https://github.com/kata-containers/kata-containers/blob/2.0-dev/docs/design/kata-api-design.md?

@YchauWang
Copy link
Author

@amshinde OK! I try to push a PR for 2.0. And 2.0 with shimV2, I am thinking about how to make some change for the 2.0-dev.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
needs-forward-port Changes need to be applied to a newer branch / repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Update the kata api design of the sandbox operation API
6 participants