Spec hash annotation #52
Conversation
afugazzotto
force-pushed
the
specHashAnnotation
branch
from
291c266
to
d59e3c7
Compare
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
… []byte instead of map. Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
afugazzotto
force-pushed
the
specHashAnnotation
branch
from
ba014fb
to
0edab78
Compare
afugazzotto
force-pushed
the
specHashAnnotation
branch
2 times, most recently
from
4cacbaa
to
316e3e3
Compare
Signed-off-by: Antonino Fugazzotto <[email protected]>
afugazzotto
force-pushed
the
specHashAnnotation
branch
from
316e3e3
to
3fcc35f
Compare
Signed-off-by: Antonino Fugazzotto <[email protected]>
…oller': prefer a domain-qualified finalizer name to avoid accidental conflicts with other finalizer writers Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
| } | ||
| } else { | ||
| } else if rolloutChildOp == RolloutChildUpdate { |
There was a problem hiding this comment.
@afugazzotto @xdevxy So, the logic in here only occurs if the spec of the child object has changed, right? I think there's some logic in here related to pausing that we need to do even if that's not the case, isn't there? Such as calling isPipelinePaused()
There was a problem hiding this comment.
Good catch! I also noticed that we need to call processPipelineStatus to update the status correctly. I'm making those change now.
There was a problem hiding this comment.
I wonder if we should only set the annotation on the Rollout spec until we truly update the object itself.
There was a problem hiding this comment.
I tend to think so, right? The annotation should represent that which has actually been deployed.
In the pause logic below we're essentially trying to update the spec. If we don't set the annotation until we've actually deployed the object to that spec, then it is okay to use the conditional "if spec is different" to determine whether to execute that logic.
There was a problem hiding this comment.
I think this is a valid point. So, it may be best to set the hash annotation only after a successful call to ApplyCRSpec? Would it make sense to set the hash annotation together with the status (ex: in processPipelineStatus)?
There was a problem hiding this comment.
So, it may be best to set the hash annotation only after a successful call to ApplyCRSpec?
yes
Would it make sense to set the hash annotation together with the status (ex: in processPipelineStatus)?
processPipelineStatus() should be called all the time
Signed-off-by: Antonino Fugazzotto <[email protected]>
Fixes #4
Modifications
Created the function
makeChildResourceFromRolloutAndUpdateSpecHashto make a new kubernetes.GenericObject based on the given rolloutObj. The function returns the child resource object ready to be created and an operation to be performed with the returned object. The operations are defined by the RolloutChildOperation constants and are: create the child resource, update the child resource, and do nothing. The returned operation will be used after this function is called to perform create/update operations or to skip create/update.Verification
Used existing unit tests to make sure PipelineRollout did not change expected behavior. Added unit tests to verify new functions correctness.
NOTE
Additional changes to improve reusability of the functions should be made. However, we should make those changes in a different PR to have smaller incremental changes.