Create default storage selection functions #15594
Conversation
openshift-merge-robot
added
size/XL
| if !ok { | ||
| return nil, nil, false, fmt.Errorf("not a ClusterPolicy") | ||
| } | ||
| return labels.Set(policy.ObjectMeta.Labels), authorizationapi.ClusterPolicyToSelectableFields(policy), policy.Initializers != nil, nil |
There was a problem hiding this comment.
Isn't this a behavior change? authorizationapi.ClusterPolicyToSelectableFields is not the same as your generic method?
There was a problem hiding this comment.
Isn't this a behavior change? authorizationapi.ClusterPolicyToSelectableFields is not the same as your generic method?
Maybe I've been staring at this too long. Don't both map "metadata.name" to the value of Name?
There was a problem hiding this comment.
Maybe I've been staring at this too long. Don't both map "metadata.name" to the value of Name?
here: https://github.com/openshift/origin/blob/master/pkg/authorization/apis/authorization/fields.go#L7 . There's only one "special" one and I think I plumbed it through properly.
|
|
||
| func (f AttrFunc) WithFieldMutation(fieldMutator FieldMutationFunc) AttrFunc { | ||
| return func(obj runtime.Object) (labels.Set, fields.Set, bool, error) { | ||
| labelSet, fieldSet, initialized, err := f(obj) |
There was a problem hiding this comment.
Slightly concerned about the performance impact on this. It doesn't look bad, but this is called on every get and watch and list call.
There was a problem hiding this comment.
Slightly concerned about the performance impact on this. It doesn't look bad, but this is called on every get and watch and list call.
I specifically made a mutator instead of an "add" for the fieldSet to avoid allocations. The fields being returned are map, map, bool, err, which should be pointer, pointer, tiny, nil.
There was a problem hiding this comment.
This looks ok for me performance wise. We have one map set more, which was maybe optimized by the compiler in the old code. But I don't think this matters.
|
@sttts You're a likely reviewer upstream. Seem reasonable? |
| attrFunc = storage.DefaultClusterScopedAttr | ||
| } | ||
| } | ||
| if e.PredicateFunc == nil { |
There was a problem hiding this comment.
now every Store gets a PredicateFunc. There shouldn't be side-effects I guess.
There was a problem hiding this comment.
now every Store gets a PredicateFunc. There shouldn't be side-effects I guess.
I don't think so. It was unconditionally used before.
|
Looked through this in detail. I also think the generic funcs plus the one exception are correct. Lgtm after half an hour staring. |
Thanks. I'll open the upstream |
deads2k
force-pushed
the
server-15-storage
branch
from
9c38d0c to
c68bed4
Compare
|
Changes look good but did you forget some unneeded attr funcs like OAuth clients, net namespace, etc? |
Forgot, no. Proof of principle, then a big pull to sweep. Upstream is tagged for merge. |
|
/retest |
|
/lgtm We have unit tests to prevent regression on field selection correct? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj No associated issue. Update pull-request body to add a reference to an issue, or get approval with 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 |
|
I had put all my faith that this PR would kick off our first postsubmits /retest |
|
/test extended_conformance_install_update |
2 similar comments
|
/test extended_conformance_install_update |
|
/test extended_conformance_install_update |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
@deads2k: The following test failed, say
Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
|
Automatic merge from submit-queue (batch tested with PRs 15594, 15161, 15619, 15628) |
Starts helping #15267 so that I never have to update a bajillion little methods that I don't care about.
@smarterclayton @liggitt @enj what do you think?