Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:You can indicate your approval by writing |
|
Hasn't fixed tests yet. Send out to seek for early feedbacks. |
pilot/pkg/kube/admit/admit.go
Outdated
There was a problem hiding this comment.
Each rule in the array of rules must refer to a valid group/version/resource tuple, e.g.
var rules []admissionregistrationv1beta1.Rule
for _, schema := range ac.options.Descriptor {
rules = append(rules, admissionregistrationv1beta.Rule{
APIGroups: []string{crd.ResourceGroup(&schema)},
APIVersion: []string{schema.Version},
Resources: []string{crd.ResourceName(schema.Plural)},
}
}
// assign `rule` in webhook.Webhooks[0].RulesThere was a problem hiding this comment.
But the field is a single Rule: instead of an array of Rules.
There was a problem hiding this comment.
Woops, I referenced the wrong type. See line 270 and admissionregistrationv1beta1.RuleWithOperations
ijsnellf
left a comment
There was a problem hiding this comment.
looks good, just remove or .gitignore the file from cloudfoundry
There was a problem hiding this comment.
add this to .gitignore so it does not accidentally get added to master?
|
@xiaolanz PR needs rebase |
226da0e to
e9da6ee
Compare
|
@xiaolanz PR needs rebase |
|
/test istio-pilot-e2e |
| var IstioAPIGroupVersion = schema.GroupVersion{ | ||
| Group: model.IstioAPIGroup, | ||
| Version: model.IstioAPIVersion, | ||
| Group: "config.istio.io", |
There was a problem hiding this comment.
can you make this a model.IstioDefaultAPIGroup and ditto for version rather than hardcode this here
despite the TODO that doesn't seem like an improvement
There was a problem hiding this comment.
I intentionally not to introduce something like model.IstioDefaultAPIGroup. Same for the version. Everything out of model package should resolve group/version from schema dynamically.
IstioAPIGroupVersion together with the strings will be gone because I plan to do client change in a separate PR.
| return model.Config{ | ||
| ConfigMeta: model.ConfigMeta{ | ||
| Type: model.MockConfig.Type, | ||
| Group: "config.istio.io", |
There was a problem hiding this comment.
same, let's have at least the same 2 constants used instead of hardcoded strings
There was a problem hiding this comment.
For test, it doesn't really matter. It can be anything. It doesn't have to be config.
After next PR, I will change it and make it more interesting...
|
I'm still bothered by having constants like Maybe the follow up PR can be combined/stacked with this one? |
|
/test istio-pilot-e2e |
|
Yes. follow up PR will resolve the redundant constants. Also, the constant int test will be different after the last PR. PTAL |
|
I didn't notice until now but this PR changed vendor do you know how that happened ? can you retrace ? we probably need a hook to check against accidental vendor changes |
|
fixed it in 57a9d1a in the future please always review the file changes before making a PR |
|
@ldemailly Thanks for fixing it. Actually I saw it but I wasn't so sure what it is. I will be more proactive next time. |
|
that vendor/ stuff is new and not obvious, so no worries, and feel free to help update |
Introducing Istio group prefix and version into each Istio config schema.
Removing Istio group. Preparing for each schema to have different apiVersion.
Addressing the first step in #3586