fix: cache handling on update #604
Conversation
csviri
requested review from
lburgazzoli and
metacosm
and removed request for
lburgazzoli
| return true; | ||
| } | ||
| String originalResourceVersion = getVersion(executionScope.getCustomResource()); | ||
| String customResourceVersionAfterExecution = getVersion(postExecutionControl |
There was a problem hiding this comment.
I don't like how we access an Optional without checking if it's present or not. If we never check for the presence, we might as well not use an Optional.
There was a problem hiding this comment.
This is quite a gray are, because we are checking implicitly above. So returning when there was no execution:
if (!postExecutionControl.customResourceUpdatedDuringExecution()) {
return true;
}
There was a problem hiding this comment.
It's not nice, but not sure how we could make it nicer.
| String customResourceVersionAfterExecution = getVersion(postExecutionControl | ||
| .getUpdatedCustomResource().get()); | ||
| String cachedCustomResourceVersion = getVersion(resourceCache | ||
| .getCustomResource(executionScope.getCustomResourceID()).get()); |
There was a problem hiding this comment.
Same here, so we know that in this case by definition there should be a custom resource in the cache. We are checking this elsewhere where it acutally might not be true.
I can comment it in the code.
There was a problem hiding this comment.
maybe the method should be changed to receive the custom resources instead of ExecutionScope & PostExecutionControl so the invocation could be closed where the check on the optional value presence are performed ??
There was a problem hiding this comment.
Will take a look. But though that just throwing an exception with orElseThrow would be also a way to go, since that situation should not happen here. And this way it's quite readable. What do you think?
There was a problem hiding this comment.
I don't have any strong opinion to be honest so either way is ok with me.
Usually I find easier to reason and test where there's not so much indirection but again, no strong opinions here :)
There was a problem hiding this comment.
pushed it with the mentioed way. I don't like it either. At some point we might want to rethink and/or refactor those core classes. To much complexity in one place :/
There was a problem hiding this comment.
pushed it with the mentioed way. I don't like it either. At some point we might want to rethink and/or refactor those core classes. To much complexity in one place :/
Well, v2 is the time to do it, I would say 😄
There was a problem hiding this comment.
@metacosm yes, but could we do it as a separate PR, since this is just thiss feature, and that would have effect on everything else.
There was a problem hiding this comment.
It seems that the serverless support will require some fundamental changes.
Co-authored-by: Chris Laprun <[email protected]>
No description provided.