Add a new annotation to temporarily bypass synk.#782
Conversation
| } | ||
|
|
||
| if current.GetAnnotations()[AnnotationIgnore] == "true" { | ||
| setOwnerRef(current, set) |
There was a problem hiding this comment.
where does setOwnerRef get called without the annotation? I only see a comment in line 800. shouldn't is get updated not matter the annotation?
There was a problem hiding this comment.
Added a comment to clarify. We're already calling setOwnerRef in applyAll. But here we're reverting to the current version.
Would it be easier to read if we return some extra flag and do the change one level up?
There was a problem hiding this comment.
it would be ugly as well but it would prevent inconsistent behavior. I see 3 places calling 'applyOne' but only for one I directly see the setOwnerRef call. (another passes nil as owner, so that one hopefully just does nothing and not remove an owner). but the first one in applyAll is unclear.
There was a problem hiding this comment.
The call chain is Apply -> applyAll -> applyOne
- applyAll calls setOwnerRef to point it to the new one
- applyOne calls setOwnerRef to revert that in thes code path
Options:
- submit as it is
- move setOwnerRef() call from applyAll to applyOne
- return something from applyOne and move the special case setOwnerRef call to applyAll
I think given the comment 1) is okay. 2) might also be okay. 3) is ugly.
There was a problem hiding this comment.
I'd leave the asymmetry for now, but I've handled all cases. I'll see if I can refactor the code next. If you prefer, I can do (2) .
This allows for a `kubectl edit` based workflow to make temporary changes, e.g. enable memory profiling.
3 participants
This allows for a
kubectl editbased workflow to make temporary changes, e.g. enable memory profiling.