Add optional revision flag

[dturn]

What are you trying to accomplish with this PR?

As part of the krane 1.0 migration we decided to add a --revision --current-sha flag.

This PR adds the flag with higher precedence than ENV["REVISION"].

I've added it to the old tasks so that transitioning to krane is easier.

closes: #605

How is this accomplished?
Add a --revision flag that is used over ENV["REVISION"]

Add a --current-sha flag and remove usage of ENV["REVISION"] in Krane (we still use that as a fallback in kubernetes-deploy and kubernetes-render)

What could go wrong?
People get confused about the priority.

[timothysmith0609]

Overall the code seems fine, but I have a few opinions on the implementation

• I find it confusing that a flag called revision ends up generating an internal template variable named current_sha: can we switch current_sha to revision to maintain a 1:1 correspondence?
• The use case of revision seems quite targeted: it appears we only use it to tag docker image versions. It seems like the reasons we use an env var to pass this information are largely historic (e.g. predating bindings). I'm ok if we have an explicit revision flag, since it, generally, controls an extremely important field (the actual image you're going to run). My preference would be to drop support for the implicit ENV var assignment so there's only one, canonical, place to declare it: if that results in no revision being supplied we'll simply fail template rendering and can provide a contextual error message for how to fix.

[KnVerey]

I find it confusing that a flag called revision ends up generating an internal template variable named current_sha: can we switch current_sha to revision to maintain a 1:1 correspondence?

We should make sure the flag and arg in our Ruby API match, at the very least. Switching the ERB variable would be a huge PITA for us, if not also other companies. Maybe we could expose both and switch our documentation to refer to revision?

it, generally, controls an extremely important field (the actual image you're going to run).

That's what we use it for internally, but technically it doesn't have to be used at all in the ERB (and I'm ok with throwing the default rendering error if you try to use the variable without giving us a revision). IIRC the one thing krane itself uses this value for is to emit statsD tags. And naturally the prefix is just sha, so a third name 🤦‍♀. That one would be easier to change to revision when we swap to the Krate prefix on our metrics, if we want to.

It seems like the reasons we use an env var to pass this information are largely historic (e.g. predating bindings).

You're likely correct that it predates bindings, but the specific historic reason is that we started as a Shipit integration, and Shipit gives you that env var by default.

My preference would be to drop support for the implicit ENV var assignment so there's only one, canonical, place to declare it

I'm also leaning this way. I don't think it causes us much toil? Like, internally we just have to switch our (still rather few) individual krane invocations to include --revision=ENV["REVISION"], right? Or am I missing something?

[dturn]

We should make sure the flag and arg in our Ruby API match, at the very least. Switching the ERB variable would be a huge PITA for us, if not also other companies. Maybe we could expose both and switch our documentation to refer to revision?

Both seems like a compromise in the wrong direction. I'd rather call the flag --current-sha

[dirceu]

Updated the PR with the following changes:

1. Change flag name to --current-sha
2. Remove the fallback to ENV["REVISION"]

[dturn]

lgtm, but turns out I can't approve a pr I created.

[timothysmith0609]

Krane allows current_sha to be passed to the deploy task as nil, but in DeprecatedDeployTask#statsd_tag we attempt to interpolate that value. This won't cause a crash, but we'll be sending 'empty' sha tags, which seems wrong.

We should either not use the sha tag when it's unavailable or provide a placeholder value (I'm leaning towards the former).

[lei-lo]

LGTM Thanks for cleaning up the include EnvTestHelpers too 🧹 ✨