Handle namespaced ExtraResources properly#537
Handle namespaced ExtraResources properly#537bobh66 wants to merge 1 commit intocrossplane-contrib:mainfrom
Conversation
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
|
Thank you very much for creating this PR. Do you happen to have an approximate release timeline for this change? We would greatly appreciate it, as we are depending on this update. |
adamwg
left a comment
There was a problem hiding this comment.
One thought on future-proofing, but no objection to merging this as-is either. Not sure if I'm an approver on this repo.
| requirements.Resources[k] = v.ToResourceSelector() | ||
| } else { | ||
| requirements.ExtraResources[k] = v.ToResourceSelector() //nolint:staticcheck // need to support Crossplane v1 | ||
| } |
There was a problem hiding this comment.
Would there be any harm in adding all required resources to Resources, but additionally including cluster-scoped resources in ExtraResources? I.e.:
| } | |
| requirements.Resources[k] = v.ToResourceSelector() | |
| if v.Namespace == "" { | |
| requirements.ExtraResources[k] = v.ToResourceSelector() //nolint:staticcheck // need to support Crossplane v1 | |
| } |
This would future-proof against ExtraResources support being dropped in some future Crossplane release.
There was a problem hiding this comment.
I did consider that, but I was afraid (maybe unnecessarily) that the GRPC message might get too big if the resource request was duplicated. But maybe that's not possible given the way the resource responses are processed. I'll take a quick look and probably just accept the suggestion.
Description of your changes
Use
requirements.Resourcesfor namespacedExtraResources.Fixes #483
I have: