-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Hook up operator build, testing, docker #20128
Hook up operator build, testing, docker #20128
Conversation
|
||
"istio.io/istio/pilot/pkg/serviceregistry" | ||
"istio.io/istio/pkg/util/protomarshal" | ||
) | ||
|
||
// defaults the user can override | ||
func defaultControlPlane() (*operatorV1alpha2.IstioControlPlane, error) { | ||
func defaultControlPlane() (*operatorV1alpha1.IstioOperator, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we going back a version here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why it went back, but this is the newer version. We unfortunately hadn't fully synced up the operator dep when we did the repo merge, so we have some extra changes like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operatorV1alpha2 is a private API, it was never officially released in istio/api.
@@ -84,4 +84,17 @@ func setupConfig(cfg *istio.Config) { | |||
cfg.Values["tracing.provider"] = "zipkin" | |||
cfg.Values["global.enableTracing"] = "true" | |||
cfg.Values["global.disablePolicyChecks"] = "true" | |||
|
|||
// TODO not needed once https://github.com/istio/istio/issues/20137 is in | |||
cfg.ControlPlaneValues = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@douglas-reid mind taking a look? This is to workaround a bug/feature introduced by a recent change in operator, not intended to change the test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me. Should we add something to filed bug about confirming that these values have been removed and the tests pass as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. It would be cool if github linked issues to code comments.. I added a comment on the issue
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for telemetry integration test changes.
@@ -84,4 +84,17 @@ func setupConfig(cfg *istio.Config) { | |||
cfg.Values["tracing.provider"] = "zipkin" | |||
cfg.Values["global.enableTracing"] = "true" | |||
cfg.Values["global.disablePolicyChecks"] = "true" | |||
|
|||
// TODO not needed once https://github.com/istio/istio/issues/20137 is in | |||
cfg.ControlPlaneValues = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me. Should we add something to filed bug about confirming that these values have been removed and the tests pass as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve for test and release group
LGTM WRT networking part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trigger my Approve
Part of #20112