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

Use strong type for container ID #15124

Merged
merged 1 commit into from
Oct 7, 2015
Merged

Conversation

timstclair
Copy link

Change all references to the container ID in pkg/kubelet/... to the
strong type defined in pkg/kubelet/container: ContainerID

The motivation for this change is to make the format of the ID
unambiguous, specifically whether or not it includes the runtime
prefix (e.g. "docker://").

@yifan-gu

cc/ @yujuhong @dchen1107 @vishh

@timstclair timstclair added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Oct 5, 2015
@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 5, 2015

@timstclair Thanks for taking this important(though uninteresting)work!! Appreciate! 🎩

LGTM.

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-bot
Copy link

k8s-bot commented Oct 5, 2015

Unit, integration and GCE e2e test build/test passed for commit a3bacfca3492a81ab9ed0d8be5576bf742c5e3ab.

@yujuhong
Copy link
Contributor

yujuhong commented Oct 6, 2015

LGTM. Thanks for taking on this!

@yujuhong yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 6, 2015
@timstclair timstclair closed this Oct 6, 2015
@timstclair timstclair reopened this Oct 6, 2015
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 7, 2015

GCE e2e build/test failed for commit a3bacfca3492a81ab9ed0d8be5576bf742c5e3ab.

@yifan-gu
Copy link
Contributor

yifan-gu commented Oct 7, 2015

Sorry, this needs a rebase since #14686 gets merged. I think we'd better hold other kubelet PRs so we can get this in first, otherwise there will be a lot of rebasing work.. /cc @kubernetes/goog-node

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2015
@yujuhong
Copy link
Contributor

yujuhong commented Oct 7, 2015

Agreed. #15051 is probably going to merge first. I will try to block the rest until this one lands.

Change all references to the container ID in pkg/kubelet/... to the
strong type defined in pkg/kubelet/container: ContainerID

The motivation for this change is to make the format of the ID
unambiguous, specifically whether or not it includes the runtime
prefix (e.g. "docker://").
@timstclair
Copy link
Author

Rebased and changed kubetypes to kubeletTypes for consistency.

@yujuhong yujuhong added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 7, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 7, 2015

GCE e2e test build/test passed for commit a26e0525652e670722827e68865cecf820ed199e.

@k8s-bot
Copy link

k8s-bot commented Oct 7, 2015

GCE e2e build/test failed for commit 551eff6.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2015
@k8s-github-robot
Copy link

Continuous integration appears to have missed, closing and re-opening to trigger it

@timstclair timstclair closed this Oct 7, 2015
@timstclair timstclair reopened this Oct 7, 2015
@dchen1107
Copy link
Member

ok to test

@k8s-bot
Copy link

k8s-bot commented Oct 7, 2015

GCE e2e test build/test passed for commit 551eff6.

@dchen1107
Copy link
Member

We have to manually merge this one since it is block a lot of node related prs. cc/ @janetkuo, oncall. Thanks!

janetkuo added a commit that referenced this pull request Oct 7, 2015
Use strong type for container ID
@janetkuo janetkuo merged commit aa307da into kubernetes:master Oct 7, 2015
@timstclair timstclair deleted the container-id branch August 31, 2016 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants