Raise error (instead of warn) when uncommitted git changes are detected

[matthewbjones]

After hanging around in the Discord for a few months, I've noticed that many beginners to Kamal experience "unexpected behavior" when it comes to making changes locally and then deploying, expecting that Kamal would've deployed their latest uncommitted changes.

Kamal would print out a warning message in yellow, but this can get lost in the sea of yellow messages and verbose output that Kamal prints out. Since the primary intended usage of Kamal is for you to first commit your changes before pushing/deploying, I believe it would be better to have the warning about ignoring uncommitted changes become an error, by default, and exit. If you purposefully want to have ignored, uncommitted local changes while pushing/deploying, you can suppress the error via --skip-uncommitted-changes-check, which results in Kamal printing out a warning in yellow but still proceeding (like it did by default prior to this change).

kamal build push error, by default

kamal build push skip error, via --skip-uncommitted-changes-check

kamal deploy error, by default

kamal deploy skip error, via --skip-uncommitted-changes-check

[matthewbjones]

Not 100% sold on the flag name, wondering if --skip-uncommitted-changes would actually be better and more specific to what Kamal is actually doing.

[nickhammond]

I like the idea, a couple of thoughts:

• The pre-build hook template has a few checks if you enable it from the sample it checks for a dirty tree https://github.com/basecamp/kamal/blob/main/lib/kamal/cli/templates/sample_hooks/pre-build.sample
• I use context: '.' when getting an app first deployed with Kamal as it allows me to iterate faster and get things going without having to continually commit to git. Once a deploy completes and is in a good spot, I usually remove the context: '.' setting.

It looks like there's actually a builder helper method to help detect if it's building from a git clone or just a context, it'd be great to utilize this as a way to raise. What you're ultimately trying to raise for is specifically when it's using a git clone, there shouldn't be a need to raise when using the local build context.

kamal/lib/kamal/configuration/builder.rb

Line 126 in 6f29d4e

def git_clone?

As far as the flag name, something with dirty in it would be good to match build dev #1357.

@djmb mentioned he'd be out this week, but it'd be good to get his thoughts on this since it'll change some of the kamal ergonomics.

[matthewbjones]

I like the idea, a couple of thoughts:

• The pre-build hook template has a few checks if you enable it from the sample it checks for a dirty tree https://github.com/basecamp/kamal/blob/main/lib/kamal/cli/templates/sample_hooks/pre-build.sample
• I use context: '.' when getting an app first deployed with Kamal as it allows me to iterate faster and get things going without having to continually commit to git. Once a deploy completes and is in a good spot, I usually remove the context: '.' setting.

It looks like there's actually a builder helper method to help detect if it's building from a git clone or just a context, it'd be great to utilize this as a way to raise. What you're ultimately trying to raise for is specifically when it's using a git clone, there shouldn't be a need to raise when using the local build context.

kamal/lib/kamal/configuration/builder.rb

Line 126 in 6f29d4e

def git_clone?

Kamal already uses this helper method to determine if to display the warning (and now the error), my check appears immediately after it: https://github.com/basecamp/kamal/pull/1409/files#diff-937ec1fc90a4c631a3f6f4095230378aa62fef153c3e1de477cfc2005dc2a7f9L22

As far as the flag name, something with dirty in it would be good to match build dev #1357.

I'll think on that some. At the moment, I think --skip-uncommitted-changes is the clearest. However, now that Kamal supports purposefully including uncommitted changes for a dev build, maybe the error message I introduced can inform the user of their two choices (use --skip-uncommitted-changes to use the most-recent commit, or build dev to make a dev build using uncommitted changes.

[matthewbjones]

Hey @djmb, just wanted to touch base and see if you had any initial feedback? We've been using this version of Kamal the last few weeks, and it's already saved us once (by spitting out an error and exiting), when we overlooked committing a local change.

[djmb]

There's more than just uncommitted changes to worry about. If you have a files that are not docker ignored but are git ignored, then they'll differ between a git clone build and a local build.

kamal build dev does warn about those but it does that by building and running a busybox image which is pretty heavyweight.

You can force an error when there are uncommitted changes via a custom pre-build hook which is what we do at 37signals. I do take the point about it being confusing for new users though.

I'd like to get to a simpler approach overall for this - I don't know what that is to be honest (and maybe there isn't one) but I think adding the flag now might make that harder to do, so I think we should leave this right now.

[matthewbjones]

@djmb yeah, I wanted to avoid touching too many places with the introduction of the flag to revert back to previous behavior, but because I wasn't sure if some users of Kamal intentionally rely on the default functionality of it to ignore local changes I felt like there needed to be a flag to force the old behavior.

It does still however, to me at least, seem like a big footgun that the default behavior is to "ignore" uncommitted local changes, and it continues to burn new users and on rare occasion, can burn even a seasoned Kamal user. Now, Kamal doesn't just "ignore" uncommitted changes, it does print out a warning message, but here's a few thoughts on that warning message:

1. The message is yellow in color - This is the same color that is used throughout the very verbose output. Yellow is used to show the docker command being executed, and there are tons, in the typical Kamal output. This makes it too easy to overlook actual warning messages.
2. The warning is one of the first things outputted - So with the verbose Kamal output, it scrolls up and away pretty quickly. When you notice that the latest deployment isn't matching your expectations, if you go looking around at the Kamal output, you're likely to be looking at the tail end of the output and the warning about uncommitted changes will not be noticed or seen.

@djmb Would you be open to separate PRs that tries to address these two points, separately? I can work on a PR that makes warning messages from Kamal a not-yellow color (or, change the docker command color to something other than yellow) I could also see if it is an easy & straightforward change to have the warning about uncommitted changes appear at the end of the Kamal output (somewhere around the Releasing the deploy lock.... and Finished all in ... seconds.