csi: add ${CLUSTER_ID} to placeholders#909
csi: add ${CLUSTER_ID} to placeholders#909openshift-merge-robot merged 1 commit intoopenshift:masterfrom
Conversation
|
/lgtm |
| operatorClient.Informer(), | ||
| deployInformer.Informer(), | ||
| } | ||
| if c.configInformers != nil { |
There was a problem hiding this comment.
why should this ever be nil? Feels like unneeded complexity.
There was a problem hiding this comment.
Some CSI drivers (operands) don't have the option to pass in a cluster ID, so their operators will let this to be nil
There was a problem hiding this comment.
Also, the docstring above tries to explain that
There was a problem hiding this comment.
call it optionalConfigInformer. The current pattern is a recipe to null pointer exceptions the next time somebody modifies this code and is not aware of this invariant.
There was a problem hiding this comment.
Renamed to optionalConfigInformer
| if err != nil { | ||
| return err | ||
| } | ||
| if infra.Status.PlatformStatus != nil { |
There was a problem hiding this comment.
this is correct? InfraName depends on another field? This at least needs a comment here. Looks like a bug now.
There was a problem hiding this comment.
Yup, we've seen this on baremetal. Added a comment
There was a problem hiding this comment.
this does not answer my question when reading this code. Why does PlatformStatus != nil influence why we care about cluster id?
There was a problem hiding this comment.
Sorry, I fat-fingered this.
Now that I look at it, this check doesn't make sense. I can just use .Status.InfrastructureName directly.
bertinatto
force-pushed
the
clusterid
branch
2 times, most recently
from
abd7534 to
025fe33
Compare
| file string, | ||
| kubeClient kubernetes.Interface, | ||
| namespacedInformerFactory informers.SharedInformerFactory, | ||
| configInformers configinformers.SharedInformerFactory, |
Some CSI Drivers need to be given a cluster ID so that they can tag volumes and snapshots they create. This is done so that the installer can filter those resources out and delete them on cluster deletion.
|
/approve @jsafrane please take a final look. |
openshift-ci-robot
added
the
approved
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto, jsafrane, sttts 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 |
6 participants
Some CSI Drivers need to be given a cluster ID so that they can tag volumes and snapshots they create.
This is done so that the installer can filter those resources out and delete them on cluster deletion.
CC @openshift/storage