Only keep existing mutation timestamp when user is replication - #186
Only keep existing mutation timestamp when user is replication#186benbarclay wants to merge 1 commit into
Conversation
| ) | ||
|
|
||
| const ( | ||
| // User account this component uses |
There was a problem hiding this comment.
"this component"? replication admission is a separate component from replication controller...
I would rephrase comment to explicitly mention replication controller.
There was a problem hiding this comment.
I view replication as an entire subsystem. The implementation is split across a controller and a couple of webhooks. I take it you see it differently?
There was a problem hiding this comment.
Fair enough, but in this case it's replication controller request to API server being intercepted by a webhook, so it seems useful to be more clear about what we do.
|
|
||
| func createAnnotationPatch(operation admissionv1beta1.Operation, newSD *comp_v1.ServiceDescriptor) ([]byte, error) { | ||
| ts, err := generateUpdated(operation, newSD) | ||
| func isReplication(userInfo authenticationv1.UserInfo) bool { |
| func generateUpdated(operation admissionv1beta1.Operation, newSD *comp_v1.ServiceDescriptor) (time.Time, error) { | ||
| if operation != admissionv1beta1.Update { | ||
| func generateUpdated(isReplication bool, operation admissionv1beta1.Operation, newSD *comp_v1.ServiceDescriptor) (time.Time, error) { | ||
| if operation != admissionv1beta1.Update || !isReplication { |
There was a problem hiding this comment.
Hmm... So this code is written in a way that if replication controller doesn't set the timestamp (why? is that possible?), we will generate a timestamp.
Maybe instead we could always parseTimestamp for replication controller and return error if annotation is missing?
There was a problem hiding this comment.
afaict the only situation we don't want to generate a timestamp is if the replication controller is doing an update on an existing thing. In all other cases we do want to generate a timestamp. Isn't that what's going on here?
There was a problem hiding this comment.
Yes, but what is the case when replication controller doesn't provide a timestamp? I guess to rephrase the requirement: replication controller must set timestamp and we don't touch it (but validate it's set). In all other cases we generate timestamp.
This way it will be easier to catch bugs in replication controller I think
There was a problem hiding this comment.
Fair call, I'll fix that up
There was a problem hiding this comment.
Also not sure if we actually still need to check for operation != admissionv1beta1.Update? I find this code hard to understand. My idea:
if isReplication {
ts, exists := newSD.Annotations[updatedKey]
if !exists {
return nil, errors.New("replication controller must set the mutation timestamp")
}
return parseTimestamp(ts)
}
return time.Now(), nil
658e048 to
6167fd6
Compare
| } | ||
|
|
||
| return parseTimestamp(ts) | ||
| return time.Now(), nil |
There was a problem hiding this comment.
Sorry, I think we may need to keep the timestamp if hash hasn't changed, e.g. when someone adds/deletes annotation or finalizer.
So you need to pass a flag if hash has changed?
2 participants
No description provided.