Skip to content
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

Fix support for dyff as external diff program #30

Merged
merged 5 commits into from
Jun 3, 2024
Merged

Fix support for dyff as external diff program #30

merged 5 commits into from
Jun 3, 2024

Conversation

timebertt
Copy link
Owner

dyff has some special integration in place that makes it compatible with kubectl diff, see homeport/dyff#149.
This is because, with KUBECTL_EXTERNAL_DIFF="dyff between", kubectl calls dyff <from> <to> between, but dyff expects dyff between <from> <to>.
kubectl diff uses different directories for from and to, which is automatically handled by dyff: https://github.com/homeport/dyff/blame/c382d5132c86d2280335f4cb71754ab20776a85a/internal/cmd/root.go#L85-L98

While kubectl revisions diff uses the same implementation for calling an external diff program like kubectl diff, the difference is that kubectl revisions diff doesn't use dedicated directories for from and to.

This PR changes the plugin to use dedicated directories as well, so that dyff works as an external diff program.
This might seem a bit hacky. Instead, one could also write a custom implementation that doesn't append but prepend args like between. However, I'm reluctant to stop reusing kubectl's implementation because it might reveal further discrepancies in the future.

The PR also adds a dedicated e2e test that ensures kubectl revisions diff works with the recommended setup for using dyff with kubectl, i.e., that it works with the same KUBECTL_EXTERNAL_DIFF setting.

@timebertt timebertt added the bug Something isn't working label Jun 3, 2024
`cat` no longer works as a dummy external diff tool, as it would be invoked with dir names instead of file names.
Use `ls` to verify the basic functionality of invoking an external diff tool and verify `--template-only=false` using dedicated tests.
@timebertt timebertt merged commit b4e86e8 into main Jun 3, 2024
3 checks passed
@timebertt timebertt deleted the dyff branch June 3, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant