maintain: change makefile to update local dev versions

[pdevine]

Summary

The version cli/server when you're building with the dev build with the makefile is pegged at 0.14.4. This can be misleading and confusing if you're expecting the actual version (e.g. right now we're currently on 0.15.2).

This change snags the current appVersion from the helm chart (although we can snag it from somewhere else if/when we move the helm chart) and patches our binary with the updated version (instead of 0.14.4).

[dnephin]

Thanks for fixing this!

I believe the underlying problem here is that release-please used to take care of updating internal/version.go to the latest released version. Now that we've removed that automation the file has the old versions in it.

I'm not sure the Makefile fix will work in the future. Our tests will need the correct internal.Version as soon as we add another API migration. Longer term I think we will need to restore the post-release automation to update this version automatically.

I think we should at least set the version in internal/version.go to something like 0.0.0. Even better would be to set set it to our released version. If we set it to the released version we wouldn't need to look it up in the Makefile, but I'm fine to keep the lookup. With that change this LGTM.

[dnephin]

Not something we can change right now, but this is a great example of why Makefiles are such a bad fit for this use case of "automating development tasks" (despite them being super widely used for that purpose).

Every single invocation of make will have to run this grep, even though most won't make use of it. I don't think there's any way around that for make.

Over time the number of variables builds up, and we end up with a very slow make because it's a bad fit for the job.

An equivalent bash script would be able to calculate this only for the task that requires it.

[mxyng]

While this is true, there are method to delay execution until the value is needed. In this case, using $$(...) instead of $(shell ...) assigns BUILDVERSION to the command itself rather than the output. When the line referencing BUILDVERSION is executed, it will create a subshell and run the grep.

This is actually used later on in make dev where the image SHA is evaluated after the image is built rather than when make is called which is what $(shell ...) will. In that case, using $(shell ...) will not have the right output, the annotation is unlikely to change, and the pod will not automatically restart

[dnephin]

Cool, I did not know about this feature of make! I still think that an equivalent bash script is going to be easier for most people to figure out, but good to know that there are ways to defer execution with make.

[pdevine]

I went ahead and set the version to 0.0.0. Setting the version to the current version is just going to go out of date the second we release another version of the product, so this should work "for now", although we definitely need a more permanent fix.

[mxyng]

Instead of these changes, I would rather see internal.Version set to a dummy value like 99.99.99999 to indicate a development version. This has a few benefits, the primary one being simplicity. Since the real version is injected during image and artifact builds, this value does nothing.

The only time this value has any effect is when image or artifacts are build locally, or during tests. By setting such a ridiculous value, it should be obvious to the user this is not a real version string. This version in particular should ensure all migrations are always run since it's (almost) always a higher semantic version.

The simplicity of this allows us to not mess with the Makefile, not mess with the Version in the code, and not worry about API migrations not being applied. We can actually remove code if the default Version is something like 99.99.99999.

I put the change I have in mind into #3294

[dnephin]

I really like the approach in #3294

When combined with go version -m ./infra (which gives us the git commit that was used to compile the binary) that seems like it should address the problem. We no longer have a misleading version when combined with go build, and we still have a way to check what version of the binary we are running.

[mxyng]

No longer necessary after #3294