Implement a method for printing diffs using delta.

[jamesbrauman]

I have found when working with complicated changes in kennel it can be difficult for me to determine if I've made the right change. Kennel has the plan and update_datadog rake tasks which output the changes that will be executed.

I've modified kennel to be able to support multiple modes for viewing diffs. This is controlled using the DIFF environment variable. If the environment variable is set to basic (or unset) the current diff viewing behaviour is retained. However if the environment variable is set to delta, kennel will use the new diff viewing mode I've implemented here.

The motivation for this new diff viewing mode is that (for me) the syntax-highlighting and colorizing make it a lot easier to understand the proposed changes to resources.

---

This new diff viewing mode is implemented using the delta tool:

• First, convert the original and proposed resource definition to JSON.
  • Ensure the keys are in the same order, otherwise the diffs will contain irrelevant changes.
• Then, execute the diff command to generate a diff between the original and proposed definitions.
• Then, execute the delta command and capture the output.
• Finally, execute the default system pager (set with the environment variable PAGER - or less if unset) and pass the output from delta to it.

I've taken special care to ensure that with this new mode to handle destinations that are not TTYs (e.g. rake plan > diff.txt). In this scenario, the diffs produced by the diff command will be output to instead, and ANSI color codes will not be output.

If the delta command does not exist in the path, an error is raised. Delta must be installed separately.

---

This screenshot shows the delta diff viewing mode in action. Please excuse the redactions of sensitive information.

---

At this time I'm looking for feedback on this before I spend the time writing tests etc. Thanks to @zdrve for the previous review on #179,

[jamesbrauman]

It's possible that the diff classes defined inside Plan (Plan::Diff and Plan::Diff::Object) are useful in other places in the codebase, but I'm not too familiar with the rest and wanted to keep my changes relatively isolated.

[zdrve]

Thanks for the detailed description and screenshot :-) I'd like to see this split into two PRs:

1. Have Plan be a separate data object with nicer encapsulation, so it can safely be used in various ways. Keep everything else, including plan visualisation, the same. In other words: no functional changes.
2. Add a new visualisation.

I could use part (1) in some other work I've been doing (so I might even do part 1 myself, unless you get there first).

[zdrve]

How portable is this across different platforms?

[zdrve]

Suggested change

	case ENV["DIFF"]
	when "basic"
	klass = Plans::Viewers::Basic
	when "delta"
	klass = Plans::Viewers::Delta
	when nil
	klass = Plans::Viewers::Basic
	case ENV.fetch("DIFF", "basic")
	when "basic"
	klass = Plans::Viewers::Basic
	when "delta"
	klass = Plans::Viewers::Delta

[zdrve]

Both these error messages are a bit cryptic, they don't tell the user how they can fix the problem. I wonder if we can provide a bit more help to them?

[zdrve]

I thought pretty_generate never added a trailing newline, no?

[zdrve]

This seems like a lot of code to change without test coverage.

[grosser]

I like the idea, the overhead of running an external program is pretty high though.

I looked into alternatives and found differ it might be a good replacement for the plain diff, but it's not as nice as delta for sure.

There is also https://github.com/halostatue/diff-lcs but I did not see a good example of it producing ascii colors, might also be worth looking into how rspec/minitest produce their diffs since they solve the same problem and might have a simple & native solution.

If we can somehow extract 'diff printing' in an initial PR (to make it pluggable and add alternatives later), that would reduce the scope of the PR and make it easy to test + merge.

[grosser]

I got this when using DIFF=delta

It taking over the whole screen until you quit is a little intrusive.

here is with diff #182

and it also works with delta

but I'm not really a fan on the coloring, maybe it's just too much

[grosser]

and here is diff-lcs

... so far my favorite. also used by rspec and thor so 🤞
once that's merged we can revisit if delta adds enough benefit to be worth the extra overhead :)

[jamesbrauman]

@grosser @zdrve Thanks for the reviews and feedback. I'll take into account how I might split this into separate parts. Hopefully I have time to work on this some more this week.

As a side note I did look into using diff-lcs but wasn't able to understand the license - it looks like it is licensed with both MIT and GPL v2. I did not find any other libraries that would be suitable.

[grosser]

dual-licensed so you can pick whatever you want rspec also uses it so it can't be an issue, otherwise half of ruby open-source world would be "GPL v2" also we use the library and do not modify so that's also not an issue

…

On Sun, Sep 4, 2022 at 6:31 PM James Brauman ***@***.***> wrote: @grosser <https://github.com/grosser> @zdrve <https://github.com/zdrve> Thanks for the reviews and feedback. I'll take into account how I might split this into separate parts. As a side note I did look into using diff-lcs but wasn't able to understand the license <https://github.com/halostatue/diff-lcs/blob/main/License.md> - it looks like it is licensed with both MIT and GPL v2. — Reply to this email directly, view it on GitHub <#180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACYZ6QZT2KYREVZ3VJT4TV4VEPXANCNFSM6AAAAAAQDAECMY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[grosser]

merged #183 lmk if you still think it's worth adding more diff methods
the current one is all I ever needed :D