Support optional resources #54
Conversation
Both these functions traverse the top level config's identity providers, make it one so only one traversal is performed.
The Add functions need to return the path on disk and not the name of the resource. AddConfigMap was updating the wrong map. Use SecretNameReference and ConfigMapNameReference to increase type safety and reduce boilerplate.
Signed-off-by: Monis Khan <mkhan@redhat.com>
openshift-ci-robot
added
the
approved
openshift-ci-robot
added
the
size/L
|
/retest Failure is unrelated. |
stlaz
left a comment
stlaz
left a comment
There was a problem hiding this comment.
Thanks for fixing my weird mistakes. I have a few comments on how to make this a bit cleaner.
| func newSourceDataIDPSecret(index int, secretName, idpType string) (string, sourceData) { | ||
| dest := getIDPName(index, secretName, idpType) | ||
| // sourceData which describes the volumes and mount volumes to mount the secret to | ||
| func newSourceDataIDPSecret(index int, secretName configv1.SecretNameReference, key string) (string, sourceData) { |
pkg/operator2/configsync.go
Outdated
| return data.path | ||
| } | ||
|
|
||
| func (sd *idpSyncData) AddSecretStringSource(index int, secretName configv1.SecretNameReference, key string) configv1.StringSource { |
There was a problem hiding this comment.
I'd rather if we got rid of the helpers.go file by including the original function in idp.go instead of making this to be a method of this object. Having AddSecretStringSource() method does not make much sense because you don't have special storage for SecretStringSource in idpSyncData, there are only Secrets and ConfigMaps.
| // Returns the path for the Secret | ||
| func (sd *idpSyncData) AddSecret(index int, secretName configv1.SecretNameReference, key string) string { | ||
| func (sd *idpSyncData) AddSecret(index int, secretName configv1.SecretNameReference, key string, optional bool) string { | ||
| if optional && len(secretName.Name) == 0 { |
There was a problem hiding this comment.
Validation should precede these additions, here I'd personally rather only do
if len(secretName.Name) == 0 {
return ""
}
It's for a simple reason - you'll need to return errors, too, which is something you're not doing here. Otherwise the problem we have now when optional field is unset would now still be present for required fields with unset values.
Proposal:
Use just the condition mentioned above. That will effectively make all such fields optional. I prepared some preliminary validation code for our CRDs for origin, I need to re-check it and then post it, then I'll get to validation on the operator side.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, stlaz The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I'll create a new PR with the suggested changes, those are non-breaking issues anyway. |
4 participants
Builds on #28 and fixes various bugs.
/assign @stlaz
@stlaz review the new commits (your commits are unchanged). I have manually tested this.