Add diff logging #191
Merged
Add diff logging #191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dhaiducek
force-pushed
the
9072-diff-log-option
branch
from
9d989d3
to
288d254
Compare
|
Particularly special thanks to @JustinKuli for the initial legwork on this PR. 😄 |
dhaiducek
force-pushed
the
9072-diff-log-option
branch
11 times, most recently
from
a86850f
to
ff59b81
Compare
|
/hold Okay, this is ready for review now! |
gparvin
reviewed
Jan 23, 2024
|
This looks good. I only had one minor comment. Thanks Dale! |
dhaiducek
force-pushed
the
9072-diff-log-option
branch
from
ff59b81
to
2bd73a2
Compare
mprahl
reviewed
Jan 24, 2024
These tests are flaky on the minimum K8s version: - operator policy compliance check - alternative kubeconfig in hosted mode Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
mprahl
reviewed
Jan 24, 2024
mprahl
reviewed
Jan 24, 2024
dhaiducek
force-pushed
the
9072-diff-log-option
branch
3 times, most recently
from
d717eb7
to
cd5d597
Compare
mprahl
previously approved these changes
Jan 24, 2024
mprahl
reviewed
Jan 25, 2024
mprahl
reviewed
Jan 25, 2024
dhaiducek
force-pushed
the
9072-diff-log-option
branch
10 times, most recently
from
19299b9
to
691d175
Compare
dhaiducek
force-pushed
the
9072-diff-log-option
branch
from
691d175
to
0a3a654
Compare
mprahl
reviewed
Jan 26, 2024
mprahl
previously approved these changes
Jan 26, 2024
Adds a `recordDiff` enum parameter to the `ConfigurationPolicy`. When set to `Log`, it uses the `go-difflib` package to compare the YAML marshalled into strings. While `go-difflib` is unmaintained, it's extensively imported, in particular by the `stretchr/testify` package here. For simplicity, the diff for objectDefinitions without a name specified are not logged. ref: https://issues.redhat.com/browse/ACM-9072 Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
dhaiducek
force-pushed
the
9072-diff-log-option
branch
from
0a3a654
to
baface8
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 2b3fab1
into
open-cluster-management-io:main
4 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
recordDiffenum parameter to theConfigurationPolicy. When set toLog, it uses thego-difflibpackage to compare the YAML marshalled into strings, with unified output similar to thediffcommand in the shell. Whilego-difflibis unmaintained, it's extensively imported, in particular by thestretchr/testifypackage here.For simplicity, the diff for objectDefinitions without a name specified are not logged.
Further enhancements could be made by making the diff object-aware. For instance, the ArgoCD diff goes so far as to hack around the Kubernetes library that implements
apply. However, I think this is a good, simple implementation as far as an MVP goes and is likely more performant than an object-aware implementation.ref: https://issues.redhat.com/browse/ACM-9072