Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Migrate code to use IstioOperator API in istio/api #713

Merged
merged 40 commits into from
Jan 8, 2020

Conversation

ostromart
Copy link
Contributor

@ostromart ostromart commented Dec 21, 2019

See the corresponding API change:
istio/api#1223

@ostromart ostromart requested a review from a team as a code owner December 21, 2019 01:03
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Dec 21, 2019
@istio-testing istio-testing added needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 21, 2019
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 21, 2019
1. [Manifest creation](#manifest-creation) code. User settings are overlaid on top of the
selected profile values and passed to a renderer in the Helm library to create manifests. Further customization on the
created manifests can be done through overlays.
1. [CLI](#cli) code. CLI code shares the `IstioControlPlaneSpec` API with
1. [CLI](#cli) code. CLI code shares the `IstioOperatorSpec` API with
Copy link
Member

Choose a reason for hiding this comment

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

Dumb question now, but isn't this used for istioctl as well?

@@ -1,67 +1,60 @@
apiVersion: install.istio.io/v1alpha2
Copy link
Member

Choose a reason for hiding this comment

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

if we are changing the API don't we need to increment the api version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is a brand new API - just updated PR to reflect that.

Copy link
Contributor

@richardwxn richardwxn left a comment

Choose a reason for hiding this comment

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

overall looks good to me, will take another detailed look again next week

README.md Outdated
K8s settings like resources, auto scaling, pod disruption budgets and others defined in the
[KubernetesResourceSpec](https://github.com/istio/operator/blob/905dd84e868a0b88c08d95b7ccf14d085d9a6f6b/pkg/apis/istio/v1alpha2/istiocontrolplane_types.proto#L411)
[KubernetesResourceSpec](https://github.com/istio/api/blob/7791470ecc4c5e123589ff2b781f47b1bcae6ddd/mesh/v1alpha1/component.proto#L103)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you point the link to master instead

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 30, 2019
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Dec 30, 2019
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 3, 2020
Copy link
Contributor

@richardwxn richardwxn left a comment

Choose a reason for hiding this comment

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

@ostromart is it ready for review again? Hope this PR can be merged soon first to unblock other PRs.

@ostromart
Copy link
Contributor Author

/retest

@@ -1,18 +1,4 @@
apiVersion: install.istio.io/v1alpha2
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the apiVersion be operator.istio.io/v1alpha1?

@@ -1,18 +1,38 @@
apiVersion: install.istio.io/v1alpha2
Copy link
Member

Choose a reason for hiding this comment

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

also here, should be operator.istio.io/v1alpha1?

@elfinhe
Copy link
Member

elfinhe commented Jan 8, 2020

we should block the master from merging other PRs before we merge this one.

@istio-testing
Copy link

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

Test name Commit Details Rerun command
e2e_operator f061a7a link /test e2e_operator

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants