encryption provider #347
encryption provider #347
Conversation
|
/assign @sttts |
p0lyn0mial
force-pushed
the
with-encryption-provider
branch
2 times, most recently
from
4e64656
to
0f15957
Compare
| } | ||
| if changed, newEncryptedGRsToManageSet := haveGRsChanged(p.currentEncryptedGRs, newEncryptedGRsToManage); changed { | ||
| p.eventRecorder.Eventf("EncryptedGRsChanged", "The new GroupResource list this operator will manage is %v", newEncryptedGRsToManageSet.List()) | ||
| p.currentEncryptedGRs = newEncryptedGRsToManage |
There was a problem hiding this comment.
this is wrong spot. Also state like this is an anti-pattern. Side-effects from a getter like this are very unexpected.
There was a problem hiding this comment.
which getter? the value is read from the cache and won't change once set.
There was a problem hiding this comment.
the state (currentEncryptedGRs) was introduced to emit an event only once it has changed.
There was a problem hiding this comment.
this is wrong spot.
why ? we assign to currentEncryptedGRs only when it has changed.
There was a problem hiding this comment.
Wasn't clear with my comment. Emitting events from a getter like this seems to be wrong.
There was a problem hiding this comment.
wouldn't it be nice to know that the list have changed (debuggability)?
frequent events might indicate errors.
p0lyn0mial
force-pushed
the
with-encryption-provider
branch
from
0f15957
to
da7f64a
Compare
|
/retest |
| @@ -67,37 +61,18 @@ func (p *encryptionProvider) EncryptedGRs() []schema.GroupResource { | |||
| return p.allEncryptedGRs // case 1 - we are in charge | |||
| } | |||
|
|
|||
| // case 2 - CAO is in charge, reduce the list | |||
There was a problem hiding this comment.
we could make the reduced list to be static too
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p0lyn0mial, sttts 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-robot
added
the
approved
No description provided.