-
Notifications
You must be signed in to change notification settings - Fork 214
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
feat: print diff resource as yaml #2542
feat: print diff resource as yaml #2542
Conversation
Signed-off-by: 10000-ki <[email protected]>
log.debug("Pruned actual: \n {} \n desired: \n {} ", prunedActual, desiredMap); | ||
var actualYml = objectMapper.asYaml(prunedActual); | ||
var desiredYml = objectMapper.asYaml(desiredMap); | ||
log.debug("Pruned actual: \n {} \n desired: \n {} ", actualYml, desiredYml); |
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.
It is possible to use a library or write code to compare YAML files. However, as the size of the YAML file increases, the cost of processing grows. Therefore, I have not added this functionality here and simply focused on outputting the data.
If deemed necessary, I can consider adding it in the future.
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'm not sure that the sorting would make that much of a difference compared to serializing to YAML in the first place…
Signed-off-by: 10000-ki <[email protected]>
As mentioned above, users can configure Jackson to sort field keys when logging YAML files by enabling the However, I'm not sure if this should be supported directly within |
@@ -104,7 +104,9 @@ public boolean matches(R actual, R desired, Context<?> context) { | |||
removeIrrelevantValues(desiredMap); | |||
|
|||
if (LoggingUtils.isNotSensitiveResource(desired)) { | |||
log.debug("Pruned actual: \n {} \n desired: \n {} ", prunedActual, desiredMap); | |||
var actualYaml = objectMapper.asYaml(prunedActual); |
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 would protect these calls with a check to whether debug is enabled so as to not incur the YAML serialization cost in that situation.
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 would protect these calls with a check to whether debug is enabled so as to not incur the YAML serialization cost in that situation.
yes it would be better thank you, i will fix it
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.
Signed-off-by: 10000-ki <[email protected]>
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.
Thank you!
Signed-off-by: 10000-ki <[email protected]>
Signed-off-by: 10000-ki <[email protected]>
var desiredYaml = serialization.asYaml(desiredMap); | ||
log.debug("Pruned actual yaml: \n {} \n desired yaml: \n {} ", actualYaml, desiredYaml); | ||
} else { | ||
log.debug("Pruned actual map: \n {} \n desired map: \n {} ", prunedActualMap, desiredMap); |
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.
Not sure if I follow this. "If debug is not enabled we log something on debug" ?
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 yes, missed that, though there should actually be no output since debug
is checked internally (iirc) but it does make sense to remove the else branch altogether, indeed.
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.
we remain the debug logging regardless of whether the debug mode was enabled.
Would it be better to remove the else
block?
Although the cost of the else
block itself doesn't seem significant, I'd like to hear your thoughts.
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.
What I mean is that the else block never logs any message, since only called if no debug is enabled but logs on debug level.
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.
Signed-off-by: 10000-ki <[email protected]>
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.
LGTM
Thank you @10000-ki ! |
Hi @10000-ki. Thanks for the PR! I think printing the whole actual and desired resource is a bit inconvenient since we still need to copy and paste it into other tools to get the real diff. How about improving the logging logic to directly print the actual diff, like shown below?
I can work on this if you think the change would be helpful. |
Hi @bachmanity1, I was kind of thinking the same thing but seeing the resource in YAML format is already helpful. We'd happily consider a PR to output the actual diffs! Keep in mind, though, that we want to keep this output as efficient as possible so it should probably be only activated via a flag. |
oh sure great!! |
resolved: #2492
AS-IS
Output diff data in text format
TO-BE
Output diff data in yml format
If the user sorts fields through Jackson settings
.enable(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS)
it will look much better because the keys will be displayed in sorted order.