refine names and errors #25
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@deads2k: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
| @@ -37,11 +37,11 @@ var ( | |||
| _ AllDesiredMutationsGetter = &applyConfiguration{} | |||
| ) | |||
|
|
|||
| func ValidateAllDesiredMutationsGetter(allDesiredMutationsGetter AllDesiredMutationsGetter, allAllowedOutputResources *libraryoutputresources.OutputResources) error { | |||
| func ValidateUnfilteredMutations(allDesiredMutationsGetter AllDesiredMutationsGetter, allAllowedOutputResources *libraryoutputresources.OutputResources) []error { | |||
| errs := []error{} | |||
There was a problem hiding this comment.
nit: move closer to where it is used.
| return errs | ||
| } | ||
|
|
||
| func ValidateAllDesiredMutationsGetter(allDesiredMutationsGetter AllDesiredMutationsGetter, allAllowedOutputResources *libraryoutputresources.OutputResources) []error { |
There was a problem hiding this comment.
The previous function (ValidateUnfilteredMutations) checks if a request is on the allowed list for all resources.
Shouldn't this function instead check if mutations for the given cluster type are on the allowed list specifically for that cluster type?
There was a problem hiding this comment.
ok, nvm, pruning/filtering is done at
| errs := []error{} | ||
|
|
||
| if allDesiredMutationsGetter == nil { | ||
| return fmt.Errorf("applyConfiguration is required") | ||
| return []error{fmt.Errorf("applyConfiguration is required")} | ||
| } | ||
|
|
||
| allMutationRequests := []manifestclient.SerializedRequestish{} |
There was a problem hiding this comment.
I think that allMutationRequests contains duplicate entries based on the code at
| errs = append(errs, fmt.Errorf("mutations for %q reported type=%q", clusterType, desiredMutationsGetter.GetClusterType())) | ||
| } | ||
|
|
||
| allMutationRequests = append(allMutationRequests, desiredMutationsGetter.Requests().AllRequests()...) |
There was a problem hiding this comment.
It looks like we're just appending to this slice, but not using it for anything else. Can it be dropped?
There was a problem hiding this comment.
This function duplicates logic from ValidateUnfilteredMutations, right? We could extract the shared code into a helper and use it in both places.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
No description provided.