move v1alpha2 to networking/v1alpha3 crd instead of config by GregHanson · Pull Request #3912 · istio/istio

GregHanson · 2018-03-02T20:16:23Z

No description provided.

istio-merge-robot · 2018-03-02T20:16:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: ldemailly

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

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

Details

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

OWNERS

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

GregHanson · 2018-03-02T20:17:08Z

related to discussion here:
#3906

does not fully fix, because crd client still needs to be able to handle apiversion v1alpha1 and v1alpha3 at same time

xiaolanz · 2018-03-02T21:14:30Z

install/kubernetes/helm/istio/charts/pilot/templates/crds.yaml

 kind: CustomResourceDefinition
 metadata:
-  name: destinationrules.config.istio.io
+  name: destinationrules.networking.istio.io


need change version v1alpha2 -> v1alpha3 to align with proto message package path.

ldemailly · 2018-03-03T04:28:15Z

just curious, it wasn't possible to do both changes in 1 ? (why first alpha2->alpha3 and then package change?)

istio-merge-robot · 2018-03-03T04:28:59Z

@GregHanson PR needs rebase

codecov · 2018-03-05T16:54:08Z

Codecov Report

❗ No coverage uploaded for pull request base (master@18a20f9). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #3912   +/-   ##
========================================
  Coverage          ?     76%           
========================================
  Files             ?     297           
  Lines             ?   26945           
  Branches          ?       0           
========================================
  Hits              ?   20293           
  Misses            ?    5354           
  Partials          ?    1298

Impacted Files	Coverage Δ
pilot/pkg/model/config.go	`61% <ø> (ø)`
pilot/pkg/model/gateway.go	`100% <100%> (ø)`
pilot/pkg/proxy/envoy/v2/mesh.go	`76% <100%> (ø)`

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 18a20f9...9a43915. Read the comment docs.

xiaolanz

It would be nice to add a few more testcases for the new configs in
pilot/test/mock/config.go
func CheckIstioConfigTypes()

xiaolanz · 2018-03-05T19:33:26Z

pilot/pkg/model/config.go

 	// NamespaceAll is a designated symbol for listing across all namespaces
 	NamespaceAll = ""
+
+	configGroup = "config"


do not define constants here. it doesn't help to clarify the model. Please write "networking" or "config" in each proto schema.

i plan to remove istioAPIVersion once everything is done.

ijsnellf

please undo the RDSv2 rename

ijsnellf · 2018-03-05T19:55:30Z

tests/e2e/tests/pilot/pilot_test.go

-	flag.BoolVar(&config.V1alpha2, "v1alpha2", config.V1alpha2, "Enable / disable v1alpha2 routing rules.")
-	flag.BoolVar(&config.RDSv2, "rdsv2", false, "Enable RDSv2 for v1alpha2")
+	flag.BoolVar(&config.V1alpha3, "v1alpha3", config.V1alpha3, "Enable / disable v1alpha3 routing rules.")
+	flag.BoolVar(&config.RDSv3, "rdsv3", false, "Enable RDSv3 for v1alpha3")


RDSv2 != v1alpha2/3. I think the v2 refers to the using the Envoy v2 API, not to v1alpha2/3. This should stay RDSv2 in all references.

did some test failed? (I would hope so but...)

ijsnellf · 2018-03-05T19:55:48Z

tests/e2e/tests/pilot/util/config.go

-	V1alpha2              bool
-	RDSv2                 bool
+	V1alpha3              bool
+	RDSv3                 bool


ijsnellf · 2018-03-05T19:55:55Z

tests/e2e/tests/pilot/util/environment.go

 	Zipkin                 bool
 	UseAdmissionWebhook    bool
-	RDSv2                  bool
+	RDSv3                  bool


ijsnellf · 2018-03-05T19:56:02Z

tests/e2e/tests/pilot/util/environment.go

 		MixerCustomConfigFile:  e.MixerCustomConfigFile,
 		CABundle:               e.CABundle,
-		RDSv2:                  e.Config.RDSv2,
+		RDSv3:                  e.Config.RDSv3,


ijsnellf

looks fine once @xiaolanz comments are addressed and tests pass

GregHanson · 2018-03-05T22:48:46Z

@xiaolanz PTAL at most recent commits for requested changes

GregHanson · 2018-03-06T00:25:11Z

/retest

GregHanson · 2018-03-06T01:11:41Z

/test e2e-bookInfo

GregHanson · 2018-03-06T01:30:52Z

/test istio-unit-tests

GregHanson · 2018-03-06T01:38:16Z

/test istio-pilot-e2e

GregHanson · 2018-03-06T13:47:35Z

/test e2e-bookInfo

GregHanson · 2018-03-06T13:47:52Z

/test istio-pilot-e2e

istio-testing · 2018-03-06T14:09:58Z

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

Test name	Commit	Details	Rerun command
prow/e2e-bookInfoTests.sh	`9a43915`	link	`/test e2e-bookInfo`
prow/istio-pilot-e2e.sh	`9a43915`	link	`/test istio-pilot-e2e`

Details

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.

sebastienvas · 2018-03-06T17:40:29Z

It seems that istio-pilot-e2e has been consistently failing since this PR.

douglas-reid · 2018-03-06T19:36:13Z

Yes, as has e2e-bookInfoTests.

@GregHanson can you take a look ?

move v1alpha2 to networking crd instead of config

a6a5227

GregHanson requested review from a team, ijsnellf and xiaolanz March 2, 2018 20:16

googlebot added the cla: yes label Mar 2, 2018

xiaolanz reviewed Mar 2, 2018

View reviewed changes

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 3, 2018

merged with master

0a47370

istio-merge-robot removed the needs-rebase Indicates a PR needs to be rebased before being merged label Mar 5, 2018

moved to v1alpha3 api version

a03387c

GregHanson requested a review from a team March 5, 2018 19:29

xiaolanz suggested changes Mar 5, 2018

View reviewed changes

xiaolanz changed the title ~~move v1alpha2 to networking crd instead of config~~ move v1alpha2 to networking/v1alpha3 crd instead of config Mar 5, 2018

ijsnellf suggested changes Mar 5, 2018

View reviewed changes

revert back to rdsv2 as unrelated to v1alpha3 changes

2745bd3

ijsnellf approved these changes Mar 5, 2018

View reviewed changes

GregHanson added 2 commits March 5, 2018 16:18

remove group version constants from model.go

f256efb

added config test cases for new types

9992fd9

GregHanson added the kind/fixit label Mar 5, 2018

lint fix

605b781

bug fix

9a43915

xiaolanz approved these changes Mar 5, 2018

View reviewed changes

GregHanson merged commit eebf7d4 into istio:master Mar 6, 2018

nmittler mentioned this pull request Mar 6, 2018

Test Failure (flakiness): e2e-bookInfoTests (VersionMigration, DbRoutingMysql, ExternalDetailsService) #4017

Closed

GregHanson deleted the networking-config branch January 10, 2023 14:19

Conversation

GregHanson commented Mar 2, 2018

Uh oh!

istio-merge-robot commented Mar 2, 2018

Uh oh!

GregHanson commented Mar 2, 2018

Uh oh!

xiaolanz Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly commented Mar 3, 2018

Uh oh!

istio-merge-robot commented Mar 3, 2018

Uh oh!

codecov bot commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xiaolanz left a comment

Choose a reason for hiding this comment

Uh oh!

xiaolanz Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijsnellf left a comment

Choose a reason for hiding this comment

Uh oh!

ijsnellf Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

ldemailly Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

ijsnellf Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

ijsnellf Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

ijsnellf Mar 5, 2018

Choose a reason for hiding this comment

Uh oh!

ijsnellf left a comment

Choose a reason for hiding this comment

Uh oh!

GregHanson commented Mar 5, 2018

Uh oh!

GregHanson commented Mar 6, 2018

Uh oh!

GregHanson commented Mar 6, 2018

Uh oh!

GregHanson commented Mar 6, 2018

Uh oh!

GregHanson commented Mar 6, 2018

Uh oh!

GregHanson commented Mar 6, 2018

Uh oh!

GregHanson commented Mar 6, 2018

Uh oh!

istio-testing commented Mar 6, 2018

Uh oh!

sebastienvas commented Mar 6, 2018

Uh oh!

douglas-reid commented Mar 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

codecov bot commented Mar 5, 2018 •

edited

Loading

xiaolanz Mar 5, 2018 •

edited

Loading