-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add metadata showing upcoming changes on debug'd resources #254
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do think this is an interesting idea. just a few comments to address
lib/BaseController.js
Outdated
if (objectPath.get(file, ['metadata', 'annotations']) === null) { | ||
objectPath.set(file, ['metadata', 'annotations'], {}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would still be necessary since this.reconcileFields(file, lastApplied);
actually is going through and nulling
values. So its a guard against trying to write to annotations after reconcileFields nulled it.
lib/BaseController.js
Outdated
if (objectPath.get(file, ['metadata', 'annotations']) === null) { | ||
objectPath.set(file, ['metadata', 'annotations'], {}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isnt entirely necessary, but it doesnt really hurt to make sure annotations is not null before trying to write to it
lib/BaseController.js
Outdated
@@ -556,9 +566,6 @@ module.exports = class BaseController { | |||
|
|||
let original = clone(file); | |||
this.reconcileFields(file, lastApplied); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.reconcileFields(file, lastApplied); | |
this.reconcileFields(file, lastApplied); | |
if (objectPath.get(file, ['metadata', 'annotations']) === null) { | |
objectPath.set(file, ['metadata', 'annotations'], {}); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry.. i had a push issue... i had done these things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah i had wondered why you ignored me 😄
i just re-read your description and i missed it the first time, but it sounds like you want the pending changes to show on the resource (ie a configmap created by a mtp). But by called the patchSelf() function, you are putting the annotation on the MTP itself |
Co-authored-by: Alex Lewitt <[email protected]>
The current common model in k8s of providing a last-applied-configuration gave me an idea for addressing #156. Basically, instead of trying to find some way to expose the configuration via an API or the logs, put it on the resources now.
Yes, the con here is that you are editing the resource, but only a piece of metadata.
Anyone could write a wrapper to extract this and compare to the existing to identify what is going to change.
This does not address the following:
How cluster locking works. because it would be really good to also have this information for when a cluster is locked, cause the scope of change is much larger.
If you are doing nested dependencies, such as locking a configmap that affects a mustache template, then only the configmap would get this annotation. Not really a reasonable way to expect razee to follow that chain.