Skip to content

Conversation

xiaolanz
Copy link
Contributor

@xiaolanz xiaolanz commented Feb 28, 2018

Wrap CRD client to a set of rest clients each for one apiGroupVersion.

Modify istioctl to handle existing config.istio.io group for Mixer resources.

Addresses #3586, #3880

@xiaolanz xiaolanz requested a review from a team February 28, 2018 06:50
@xiaolanz xiaolanz changed the title Change pilot CRD client to handle multiple apiGroupVersions [WIP] Change pilot CRD client to handle multiple apiGroupVersions Feb 28, 2018
@xiaolanz xiaolanz self-assigned this Feb 28, 2018
@xiaolanz xiaolanz added the do-not-merge/hold Block automatic merging of a PR. label Feb 28, 2018
@codecov
Copy link

codecov bot commented Feb 28, 2018

Codecov Report

Merging #3830 into master will increase coverage by 1%.
The diff coverage is 74%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #3830    +/-   ##
=======================================
+ Coverage      76%     76%    +1%     
=======================================
  Files         297     297            
  Lines       27075   27257   +182     
=======================================
+ Hits        20407   20566   +159     
  Misses       5358    5358            
- Partials     1310    1333    +23
Impacted Files Coverage Δ
pilot/pkg/serviceregistry/kube/client.go 30% <ø> (ø) ⬆️
security/pkg/workload/server.go 74% <ø> (ø) ⬆️
pilot/pkg/kube/inject/inject.go 80% <ø> (ø) ⬆️
pilot/pkg/serviceregistry/consul/conversion.go 91% <ø> (ø) ⬆️
mixer/pkg/config/crd/admit.go 59% <ø> (ø) ⬆️
pilot/pkg/serviceregistry/kube/queue.go 86% <ø> (ø) ⬆️
pilot/pkg/config/kube/crd/types.go 62% <ø> (+5%) ⬆️
pilot/pkg/model/config.go 61% <ø> (ø) ⬆️
pilot/pkg/config/kube/crd/controller.go 79% <50%> (-2%) ⬇️
pilot/pkg/config/kube/crd/client.go 69% <75%> (-4%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720be7a...5a2e228. Read the comment docs.

"github.com/spf13/cobra/doc"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/runtime/serializer"
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure you run bin/fmt.sh on the changes files. Imports are unsorted.

errs = multierror.Append(errs, err)
}
return errs
}

// ConfigDescriptor for the store
func (cl *Client) ConfigDescriptor() model.ConfigDescriptor {
return cl.descriptor
d := make(model.ConfigDescriptor, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

make(type, 0, len(cl.clientset)

@xiaolanz xiaolanz requested a review from diemtvu February 28, 2018 23:21
@istio-merge-robot
Copy link

@xiaolanz PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 28, 2018
@xiaolanz xiaolanz requested a review from ayj February 28, 2018 23:21
@xiaolanz xiaolanz requested review from a team March 1, 2018 07:17
@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 1, 2018
@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 2, 2018
@xiaolanz
Copy link
Contributor Author

xiaolanz commented Mar 2, 2018

/retest

@istio-merge-robot
Copy link

@xiaolanz PR needs rebase

@istio-merge-robot istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 2, 2018
@istio-merge-robot istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 2, 2018
@xiaolanz
Copy link
Contributor Author

xiaolanz commented Mar 2, 2018

PTAL

"github.com/spf13/cobra"
"github.com/spf13/cobra/doc"
Copy link
Contributor

Choose a reason for hiding this comment

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

Newline requried between github and k8s imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to ignore this if go fmt is removing the newline.

Copy link
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

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

/lgtm

@xiaolanz xiaolanz requested a review from a team March 2, 2018 22:14
@istio-merge-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @ayj @xiaolanz

@diemtvu
Copy link
Contributor

diemtvu commented Mar 2, 2018

/lgtm

@istio-merge-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ayj, diemtvu
We suggest the following additional approvers: guptasu, yutongz

Assign the PR to them by writing /assign @guptasu @yutongz in a comment when ready.

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

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

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

@istio-testing
Copy link
Collaborator

@xiaolanz: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/istio-presubmit.sh 5a2e228 link /test istio-presubmit

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.

@xiaolanz xiaolanz merged commit aeff05e into istio:master Mar 2, 2018
diemtvu added a commit to diemtvu/istio that referenced this pull request Mar 5, 2018
diemtvu added a commit to diemtvu/istio that referenced this pull request Mar 6, 2018
diemtvu added a commit that referenced this pull request Mar 7, 2018
* Add authentication policy to istio config.

* Merge #3830

* Add IstioConfigTest for authentication policy type.

* Change Authentication policy type to authentication-policy to match with schema name. It's still a requirement.

* Change AuthenticationPolicyByDestination to return *Config.

* Patch generate.sh from https://github.com/xiaolanz/istio/blob/707afce3a0538b7cf4e6624928a605a68063b81f/pilot/pkg/config/kube/crd/generate.sh and change CRD type back to Policy.

* Lint

* Rebase

* Add tracking bug for global config

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

Successfully merging this pull request may close these issues.

8 participants