-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
CRI: general grammar/spelling/consistency cleanup #36448
Conversation
Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot ok to test" on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands will still work. Regular contributors should join the org to skip this step. If you have questions or suggestions related to this bot's behavior, please file an issue against the kubernetes/test-infra repository. |
@k8s-bot ok to test |
LGTM. Thanks for the cleanup! |
@k8s-bot gci gke e2e test thi |
857bc60
to
0dee0ee
Compare
repeated Mount mounts = 14; | ||
} | ||
|
||
message ContainerStatusResponse { | ||
// The status of the container | ||
// Status of the container> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/>/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! fixed
optional ContainerMetadata metadata = 3; | ||
// The spec of the image | ||
// Spec of the image |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a .
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
optional string container_id = 1; | ||
// Timeout in seconds to stop the container | ||
// Timeout, in seconds, to stop the container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's fine IMO and I expect this will be cleaned up when we address the TODO for what the timeout means
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine.
No semantic changes, but a lot of shuffling of docstrings to make things more consistent. In particular, standardise on the zeroth-article (i.e. prefer `// Version` to `// The version`) and ending all docstrings with periods.
@@ -537,45 +538,45 @@ message ContainerConfig { | |||
optional bool stdin_once = 13; | |||
optional bool tty = 14; | |||
|
|||
// Linux contains configuration specific to Linux containers. | |||
// Configuration specific to Linux containers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe change this to Linux specific configuration
(highlight linux instead of containers).
@@ -81,19 +82,19 @@ service ImageService { | |||
} | |||
|
|||
message VersionRequest { | |||
// The version of kubelet runtime API. | |||
// Version of the kubelet runtime API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: change kubelet runtime API
to Container Runtime Interface
for keeping consistent with other places (e.g. issues and PRs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think kubelet runtime API
is unclear, but let's not block this PR which focuses on consistency cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@jonboulle will you draw a following PR for this?
optional string version = 1; | ||
} | ||
|
||
message VersionResponse { | ||
// The version of the kubelet runtime API. | ||
// Version of the kubelet runtime API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@feiskyer re: your last three, I don't quite agree and I'd rather address them in a follow up since this is a pretty big diff as-is. |
Jenkins GCI GKE smoke e2e failed for commit 857bc602e440b314f56088c96a4a5feeb5e6391c. Full PR test history. The magic incantation to run this job again is |
This PR only changes comments; ok for post-freeze merge. |
Ack |
@yujuhong thank you :-) |
Jenkins unit/integration failed for commit 37150b6. Full PR test history. The magic incantation to run this job again is |
1 similar comment
Automatic merge from submit-queue |
No semantic changes, but a lot of shuffling of docstrings to make things
more consistent. In particular, standardise on the zeroth-article (i.e.
prefer
// Version
to// The version
) and ending all docstrings withperiods.
(This knowingly conflicts with #36446 and intentionally omits changing the
Annotations field - I'll rebase this or that respectively as necessary.)
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)