Kubectl apply and strategic merge patch using openapi #51321

mengqiy · 2017-08-25T07:18:43Z

support openapi in strategic merge patch
test openapi in strategic merge patch
kubectl apply use openapi to calculate diff be default. It will fall back to use baked-in types when openapi is not available.
test openapi in kubectl apply

kubectl apply use openapi to calculate diff be default. It will fall back to use baked-in types when openapi is not available.

/assign @apelisse

mengqiy · 2017-08-25T07:20:08Z

I will add tests for kubectl apply soon. It should not affect other code.

apelisse

That is very big, and there are clear ways to split. Would you mind splitting in smaller pieces?

apelisse · 2017-08-25T16:04:58Z

pkg/kubectl/cmd/util/openapi/document.go

@@ -111,23 +111,23 @@ func parseGroupVersionKind(s *openapi_v2.Schema) schema.GroupVersionKind {
 // Definitions is an implementation of `Resources`. It looks for
 // resources in an openapi Schema.
 type Definitions struct {
-	models    map[string]Schema


These should really really not be public. Let's see below why you need that.

apelisse · 2017-08-25T16:07:21Z

pkg/kubectl/cmd/util/openapi/errors.go

@@ -0,0 +1,43 @@
+/*


This should be done in a preliminary pull-request, along with a description. It's really hard to guess what you're trying to do and why you're doing this. (though I might have an idea :-)

Also maybe move that code to a different package?

apelisse · 2017-08-25T16:09:28Z

pkg/kubectl/cmd/util/openapi/validation/types.go

-type baseItem struct {
-	errors Errors
-	path   openapi.Path
+type baseValidationItem struct {


do we need that type if it's basically just embedding the openapi.BaseItem?

apelisse · 2017-08-25T16:10:58Z

pkg/kubectl/cmd/util/openapi/validation/types.go

@@ -81,11 +63,11 @@ func (item *mapItem) sortedKeys() []string {
 var _ ValidationItem = &mapItem{}

 func (item *mapItem) VisitPrimitive(schema *openapi.Primitive) {
-	item.AddValidationError(InvalidTypeError{Path: schema.GetPath().String(), Expected: schema.Type, Actual: "map"})
+	item.AddValidationError(openapi.InvalidTypeError{Path: schema.GetPath().String(), Expected: schema.Type, Actual: "map"})


InvalidTypeError is a validation error, I'm not sure why it's coming from openapi

I'm reusing it in my code.

apelisse · 2017-08-25T16:11:33Z

pkg/kubectl/cmd/util/openapi/validation/types.go

 }

 // mapItem represents a map entry in the yaml.
 type mapItem struct {
-	baseItem
+	baseValidationItem


do we need that type if it's basically just embedding the openapi.BaseItem?

Then this would be openapi.BaseItem

apelisse · 2017-08-25T16:12:53Z

pkg/kubectl/cmd/util/openapi/validation/types.go

-// Errors returns the list of errors found for this item.
-func (item *baseItem) Errors() []error {
-	return item.errors.Errors()
+func NewBaseValidationItem(path openapi.Path) baseValidationItem {


do we need that type if it's basically just embedding the openapi.BaseItem?

If we don't have the extra layer, then this function can be removed

apelisse

I still haven't read patch.go

apelisse · 2017-08-25T16:46:22Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/meta.go

+
+	errs := patchItem.Errors()
+	if errs != nil {
+		return nil, nil, utilerrors.NewAggregate(errs)


Looking at PatchMetaFromStruct.LookupPatchMetadata, it looks like aggregating the errors is a new feature. is this really something we want? IOW, it made sense for validation, does it make sense here? I'd rather keep it simple.

apelisse · 2017-08-25T16:48:18Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/meta.go

+	if errs != nil {
+		return nil, nil, utilerrors.NewAggregate(errs)
+	}
+	return PatchMetaFromOpenAPI{Schema: patchItem.subschema},


I don't know what subschema is yet, but that looks a little odd

OK, Now that I've seen what subschema is. I think that code belongs to types. IOW, the code that depends on the subschema should be where you know if you have a subschema, a list or a primitive.

was this addressed?

apelisse · 2017-08-25T16:49:34Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/meta.go

+}
+
+func (s PatchMetaFromOpenAPI) Elem() LookupPatchMeta {
+	arr, ok := s.Schema.(*openapi.Array)


We really shouldn't have to cast :-/

I know cast is bad, when it supposed to be visited by the interface. :-/
I will try to refactor it and I will touch k8s.io/apimachinery/third_party/forked/golang/json/fields.go to get rid of Elem().

apelisse · 2017-08-25T16:54:12Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/types.go

+type LookupPatchItem interface {
+	openapi.SchemaVisitor
+
+	Errors() []error


As mentionned earlier, I'd maybe keep that as a simple Error error.

Also note that if you make that change, everything will break in an non-obvious way because this doesn't implement a "Item" interface of some sort.

apelisse · 2017-08-25T17:04:00Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/types.go

+	Path() *openapi.Path
+}
+
+type baseLookupPatchMetaItem struct {


Same as before, not sure this is super useful

apelisse · 2017-08-25T17:21:34Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/types.go

+	*openapi.BaseItem
+}
+
+func (item *baseLookupPatchMetaItem) AddLookupPatchMetaError(err error) {


I don't think this should be necessary

apelisse · 2017-08-25T17:24:08Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/types.go

+var _ LookupPatchItem = &patchItem{}
+
+func (item *patchItem) VisitPrimitive(schema *openapi.Primitive) {
+	item.subschema = nil


oh I see, I'll comment up-there

apelisse · 2017-08-25T17:25:05Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/types.go

+}
+
+func (item *patchItem) VisitArray(schema *openapi.Array) {
+	item.AddLookupPatchMetaError(openapi.InvalidTypeError{Path: schema.GetPath().String(), Expected: "array", Actual: "kind"})


I would consider panicking here, assuming that validation would have caught that before. (and everywhere)

apelisse · 2017-08-25T17:34:10Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/meta.go

+	if s.Schema == nil {
+		return nil, &PatchMeta{}, nil
+	}
+	patchItem := NewPatchItem(key, s.Schema.GetPath())


key is odd, do you always have a key? what if you have an array?

Array should be handled by Elem().

apelisse · 2017-08-25T17:35:24Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/types.go

+	}
+
+	extensions := subschema.GetExtensions()
+	ps := extensions[patchStrategyOpenapiextensionKey]


Maybe move that to a different function: parseStrategy(subschema.GetExtensions())

k8s-reviewable · 2017-08-30T01:42:23Z

This change is

mengqiy · 2017-08-30T02:37:20Z

@apelisse Addressed most of your comments except the bazel one. I have tweak the bazel BUILD files for a while. I can't make it work. Please advice.
FYI, to review apply_test.go, using https://reviewable.kubernetes.io/reviews/kubernetes/kubernetes/51321 will be helpful.

apelisse

That is vastly better than the previous version, thank you Mengqi

apelisse · 2017-08-25T17:44:10Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go

@@ -1296,21 +1298,23 @@ func mergeMap(original, patch map[string]interface{}, t reflect.Type, mergeOptio
 		}
 		// If they're both maps or lists, recurse into the value.
 		// First find the fieldPatchStrategy and fieldPatchMergeKey.
-		fieldType, fieldPatchStrategies, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, k)
+		//fmt.Printf("schema: %#v\n", schema.(PatchMetaFromStruct).T)


apelisse · 2017-08-30T17:06:00Z

pkg/kubectl/cmd/apply.go

@@ -127,6 +128,7 @@ func NewCmdApply(baseName string, f cmdutil.Factory, out, errOut io.Writer) *cob
 	cmd.Flags().StringVarP(&options.Selector, "selector", "l", "", "Selector (label query) to filter on, supports '=', '==', and '!='.(e.g. -l key1=value1,key2=value2)")
 	cmd.Flags().Bool("all", false, "Select all resources in the namespace of the specified resource types.")
 	cmd.Flags().StringArray("prune-whitelist", []string{}, "Overwrite the default whitelist with <group/version/kind> for --prune")
+	cmd.Flags().Bool("openapi-patch", true, "If true, use openapi rather than baked-in types when calculating diff.")


Do we fall back on "baked-in" if openapi is not present or doesn't have the merge info?

Yes we do. I will update the help text.

apelisse · 2017-08-30T17:32:28Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/testing/BUILD

+
+filegroup(
+    name = "testing-openapi-spec",
+    srcs = glob([


You don't need glob.

apelisse · 2017-08-30T20:33:18Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/testing/openapi.go

+
+var (
+	mergeItemGVK = schema.GroupVersionKind{
+		Group:   "fake-group",


apelisse · 2017-08-30T20:39:48Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go

-var mergeItem MergeItem
+var (
+	mergeItem              MergeItem
+	mergeItemStructSchema  PatchMetaFromStruct  = PatchMetaFromStruct{T: GetTagStructTypeOrDie(mergeItem)}


You don't need to specify the type do you?

Can you just do

var ( // ... mergeItemStructSchema = PatchMetaFromStruct{T: GetTagStructTypeOrDie(mergeItem)} )

apelisse · 2017-08-30T20:45:35Z

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go

-		if !reflect.DeepEqual(got, expected) {
-			t.Errorf("error in test case: %s\ncannot sort object:\n%s\nexpected:\n%s\ngot:\n%s\n",
-				c.Description, mergepatch.ToYAMLOrError(c.Original), mergepatch.ToYAMLOrError(c.Sorted), jsonToYAMLOrError(got))
+	for _, schema := range schemas {


If that test fails, how do I know from the error message which variant of the schema failed?

Good point. It worth pointing out.

mengqiy · 2017-08-30T21:02:32Z

@apelisse Bazel test is happy now. Thank you soooo much for helping me figure out the bazel problem!

mengqiy · 2017-09-01T20:21:51Z

@apelisse ping for review or lgtm

mengqiy · 2017-11-22T05:00:38Z

/priority critical-urgent
Label it as critical-urgent to prevent the bot from moving out of 1.9 milestone.

mengqiy · 2017-11-22T05:15:52Z

/remove-priority important-soon

mengqiy · 2017-11-22T17:23:50Z

@thockin Can you please approve the godep change in be20a67? They are auto generated by script.

thockin · 2017-11-22T18:18:12Z

Rather than going to top-level OWNERs, we should see approval from OWNERs of those subsystem that are not yet covered.

@grodrigues3 @kubernetes/sig-contributor-experience-bugs this is an opportunity to refine the approver suggestions.

/approve

k8s-github-robot · 2017-11-22T18:19:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, liggitt, mengqiy, pwittrock, thockin

Associated issue: 55

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [liggitt,pwittrock,thockin]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [thockin]
~~staging/src/k8s.io/apimachinery/OWNERS~~ [liggitt,thockin]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt,thockin]
~~staging/src/k8s.io/client-go/OWNERS~~ [liggitt,thockin]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [thockin]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [liggitt,thockin]
~~staging/src/k8s.io/sample-controller/OWNERS~~ [thockin]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-22T18:19:17Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@apelisse @mengqiy @pwittrock @smarterclayton @sttts

Action required: This pull request requires label changes.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.

Help

k8s-github-robot · 2017-11-22T19:30:44Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-22T19:59:48Z

Automatic merge from submit-queue (batch tested with PRs 51321, 55969, 55039, 56183, 55976). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2017-11-22T20:00:43Z

@mengqiy: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	29f6d0825e5a8c87bfa988058931b43012d162de	link	`/test pull-kubernetes-bazel`
pull-kubernetes-e2e-gce-gpu	d4167cf044abfc5c2c551963c1467d1cad5dd16f	link	`/test pull-kubernetes-e2e-gce-gpu`
pull-kubernetes-e2e-gce-bazel	d4167cf044abfc5c2c551963c1467d1cad5dd16f	link	`/test pull-kubernetes-e2e-gce-bazel`
pull-kubernetes-e2e-gce-etcd3	d4167cf044abfc5c2c551963c1467d1cad5dd16f	link	`/test pull-kubernetes-e2e-gce-etcd3`
pull-kubernetes-node-e2e	`be20a67`	link	`/test pull-kubernetes-node-e2e`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

liggitt · 2017-11-30T15:06:05Z

pkg/kubectl/cmd/apply.go

+	// Try to use openapi first if the openapi spec is available and can successfully calculate the patch.
+	// Otherwise, fall back to baked-in types.
+	if p.openapiSchema != nil {
+		if schema = p.openapiSchema.LookupResource(p.mapping.GroupVersionKind); schema != nil {


is there any indication in the discovery doc that the resource supports strategic merge patch? I would not assume the mere presence of an openapi schema to mean SMP is supported.

I guess this question is related to the discussion #56606 (comment).

is there any indication in the discovery doc that the resource supports strategic merge patch?

I don't think there is an explicit indicator.
If there are patch metadata (e.g. PatchStrategy, MergeKey) in the discovery doc, then this type supports SMP. But if there is not patch metadata, the type may still support SMP and it uses the default behavior(replacing lists).

I would not assume the mere presence of an openapi schema to mean SMP is supported.

It's not clear what does SMP is supported mean.
If a type and its schema appear in the openapi schema with no patch metadada, SMP will process it with the default approach (replacing a list). It should not do anything wrong.

Did I miss anything?

Yes, it assumes every API server that publishes openapi schemas is able to accept strategic merge patches. Previously, the client only assumed that compiled-in known core kubernetes types were served by a server supporting SMP.

Let me summarize it for SMP

Before this PR:
Only compiled-in types (native k8s api resources) are supported.
No support for user-defined api resource no support for CRD.

After this PR:
There are 2 approaches:
Using baked-in types: only support native k8s api resources
Using openapi schema: support native k8s api resources and user-defined api resource. But no support for CRD.

Correct me if anything is wrong.

Using openapi schema: support native k8s api resources and user-defined api resource

It assumes user-defined api resources support strategic merge patch

But to do MIME type negotiation, we need to parse the endpoints part of openapi schema.

If we want to use openapi docs dynamically to form patch requests, I think that's what we need to do.

So this way will require much more efforts than the approach I mentioned above.
IMO it may be risky to introduce a lot of new code at this point.

I agree it is more effort, but I have a hard time accepting the "if at first you don't succeed" negotiation approach when we've already been given exactly the information we need to know what patch types are accepted

and is risky at this point

I also agree with this. Which is better at this point for this release?

attempting multiple patch types falling back to merge patch if SMP is rejected with 406?

defaulting this feature off

disabling this feature

Whatever we choose for the moment, we should work to make use of the accepted content in the next release.

Can we: send SMP if the type is registered, JMP otherwise? AFAIU that wouldn't really change the existing behavior (before that pull-request), yet exercise that code and already have some benefits?

This is a good example of why we need libraries that combine the discovery and openapi information for each resource type. We need and abstraction that hides the specific source of metadata for the resources.

@liggitt What do you recommend for 1.9? Disable or try to get a fix in?

I'm leaning to what @apelisse suggested in #51321 (comment).
It should be a trivial change with fewer than 50 lines of code change.

hi @liggitt @mengqiy about the MIME type negotiation part is that means we check the "consumes" portion of a related resource in discovery doc, if it support, then use SMP, if not use JMP. if so what we need maybe in 1.10 is to add the function as @mengqiy memtioned

But to do MIME type negotiation, we need to parse the endpoints part of openapi schema.
There is no facility to support parsing the endpoints part of openapi schema, we only have the code to parse the models part.

so all we have to do is make NewOpenAPIData also return paths and add some lookup/indication function ? @mengqiy @apelisse I would like to take a look of adding this openapi helper function if you haven't yet.

This is a good example of why we need libraries that combine the discovery and openapi information for each resource type. We need and abstraction that hides the specific source of metadata for the resources.

This also sounds interesting, but I haven't got the idea of "combine" @pwittrock could you explain more ? thanks

liggitt · 2017-12-01T19:20:32Z

Using openapi if available to compute SMP for compiled in types seems like the best we can do for 1.9 (known types can assume SMP as today, and just get more up to date schema info via SMP) In 1.10, we can expand using openapi for SMP for non-compiled-in types once we add support for checking the "consumes" portion of the openapi doc On Dec 1, 2017, at 1:59 PM, Phillip Wittrock <notifications@github.com> wrote: *@pwittrock* commented on this pull request. ------------------------------ In pkg/kubectl/cmd/apply.go <#51321 (comment)>:

createPatchErrFormat := "creating patch with:\noriginal:\n%s\nmodified:\n%s\ncurrent:\n%s\nfor:"

- switch { - case runtime.IsNotRegisteredError(err): - // fall back to generic JSON merge patch - patchType = types.MergePatchType - preconditions := []mergepatch.PreconditionFunc{mergepatch.RequireKeyUnchanged("apiVersion"), - mergepatch.RequireKeyUnchanged("kind"), mergepatch.RequireMetadataKeyUnchanged("name")} - patch, err = jsonmergepatch.CreateThreeWayJSONMergePatch(original, modified, current, preconditions...) - if err != nil { - if mergepatch.IsPreconditionFailed(err) { - return nil, nil, fmt.Errorf("%s", "At least one of apiVersion, kind and name was changed") + // Try to use openapi first if the openapi spec is available and can successfully calculate the patch. + // Otherwise, fall back to baked-in types. + if p.openapiSchema != nil { + if schema = p.openapiSchema.LookupResource(p.mapping.GroupVersionKind); schema != nil { This is a good example of why we need libraries that combine the discovery and openapi information for each resource type. We need and abstraction that hides the specific source of metadata for the resources. @liggitt <https://github.com/liggitt> What do you recommend for 1.9? Disable or try to get a fix in? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51321 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA70cjQx3dpLJVpDu5lNuxGx_pjGX6f6ks5s8EypgaJpZM4PCTsm> .

@apelisse

Automatic merge from submit-queue (batch tested with PRs 52013, 56719). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Build patch from openapi only for registered types Address the concern in #51321 (review). fixes kubernetes/kubectl#156 ```release-note NONE ``` /assign @apelisse

k8s-ci-robot assigned apelisse Aug 25, 2017

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 25, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 25, 2017

apelisse reviewed Aug 25, 2017

View reviewed changes

mengqiy force-pushed the kubectl_apply_openapi branch from fa8caaa to 29f6d08 Compare August 28, 2017 07:14

mengqiy changed the title ~~Kubectl apply openapi~~ [WIP] Kubectl apply openapi Aug 28, 2017

mengqiy force-pushed the kubectl_apply_openapi branch 3 times, most recently from 46bd7c8 to 197316b Compare August 30, 2017 01:40

mengqiy force-pushed the kubectl_apply_openapi branch from 197316b to b87a3ba Compare August 30, 2017 02:34

mengqiy changed the title ~~[WIP] Kubectl apply openapi~~ Kubectl apply openapi Aug 30, 2017

mengqiy force-pushed the kubectl_apply_openapi branch from b87a3ba to ba2960a Compare August 30, 2017 19:56

kubernetes deleted a comment from mengqiy Aug 30, 2017

apelisse reviewed Aug 30, 2017

View reviewed changes

mengqiy mentioned this pull request Aug 31, 2017

move openapi parsing to api machinery #51674

Closed

mengqiy force-pushed the kubectl_apply_openapi branch 2 times, most recently from 518d2ff to 838d471 Compare August 31, 2017 21:34

mengqiy changed the title ~~Kubectl apply openapi~~ Kubectl apply and strategic merge patch using openapi Sep 1, 2017

mengqiy mentioned this pull request Sep 1, 2017

Update vendor kube-openapi #51762

Merged

mengqiy force-pushed the kubectl_apply_openapi branch 3 times, most recently from a7519f8 to 922aecc Compare September 1, 2017 07:29

k8s-github-robot removed the milestone/removed label Nov 22, 2017

k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 22, 2017

k8s-github-robot added milestone/incomplete-labels and removed milestone/needs-attention labels Nov 22, 2017

liggitt approved these changes Nov 22, 2017

View reviewed changes

k8s-ci-robot removed the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 22, 2017

k8s-github-robot added milestone/needs-attention and removed milestone/incomplete-labels labels Nov 22, 2017

k8s-ci-robot added sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. kind/bug Categorizes issue or PR as related to a bug. labels Nov 22, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. milestone/incomplete-labels and removed milestone/needs-attention labels Nov 22, 2017

k8s-github-robot merged commit e412ad5 into kubernetes:master Nov 22, 2017

mengqiy deleted the kubectl_apply_openapi branch November 27, 2017 21:41

liggitt reviewed Nov 30, 2017

View reviewed changes

This was referenced Dec 1, 2017

Build patch from openapi only for registered types #56719

Merged

Apply should not assume that publishing openapi spec means support strategic merge patch kubernetes/kubectl#156

Closed

mfojtik mentioned this pull request Jan 4, 2018

kubectl patch --type=strategic should prompt the name of the required key #57834

Closed

Kubectl apply and strategic merge patch using openapi #51321

Kubectl apply and strategic merge patch using openapi #51321

Conversation

mengqiy commented Aug 25, 2017 • edited

mengqiy commented Aug 25, 2017

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-reviewable commented Aug 30, 2017

mengqiy commented Aug 30, 2017

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Aug 30, 2017

mengqiy commented Sep 1, 2017

mengqiy commented Nov 22, 2017

mengqiy commented Nov 22, 2017

mengqiy commented Nov 22, 2017

thockin commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-github-robot commented Nov 22, 2017

k8s-ci-robot commented Nov 22, 2017 • edited

liggitt Nov 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse Dec 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang Dec 3, 2017 • edited

Choose a reason for hiding this comment

liggitt commented Dec 1, 2017 via email

mengqiy commented Aug 25, 2017 •

edited

k8s-ci-robot commented Nov 22, 2017 •

edited

liggitt Nov 30, 2017 •

edited

apelisse Dec 1, 2017 •

edited

shiywang Dec 3, 2017 •

edited