[payment] don't rely on VERSION env var

[JanKoehnlein]

Fixes #5187

[JanKoehnlein]

/schedule
/assign

[JanKoehnlein]

/cc @jankeromnes

[jankeromnes]

Hmm, I don't feel very legitimate in reviewing this change, as I don't see what sort of side-effects it could have.

Still, if this change is urgent, I'll approve it to unblock it. I guess if it builds & deploys fine, it means there are no deal-breaking problems.

If I understand correctly, you're moving version from AbstractComponentEnv into Env, in order to no longer have it in Config (what a terribly ambiguous class name 🙈).

Looking through the code, I couldn't spot any obvious dangers, except maybe for this part which looks supicious to me (because it seems to somehow cast Config to Env and then call fromEnv, which relies on .version to exist):

gitpod/components/server/src/container-module.ts

Lines 86 to 89 in 6f15a43

	bind(Config).toDynamicValue(ctx => {
	const env = ctx.container.get<Env>(Env);
	return EnvConfig.fromEnv(env);
	}).inSingletonScope();

But maybe this is actually fine and I just don't understand this code.

---

An alternative fix could be to not move version, but give a default value as second parameter to getEnvVar (thus the call wouldn't fail even if VERSION isn't set).

For example like so (still in AbstractComponentEnv):

-    readonly version: string = getEnvVar('VERSION');
+    readonly version: string = getEnvVar('VERSION', 'unknown');

/hold

[roboquat]

LGTM label has been added.

Git tree hash: 7d61502ede12f9ab6f28417d2c5ffd942ae01e9a

[roboquat]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jankeromnes

Associated issue: #5187

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• components/gitpod-protocol/OWNERS [jankeromnes]
• components/server/OWNERS [jankeromnes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[JanKoehnlein]

Thanks @jankeromnes for the review. I think you are very legitimate to judge this change.

except maybe for this part which looks suspicious to me (because it seems to somehow cast Config to Env and then call fromEnv, which relies on .version to exist)

That should be fine, because a) the source is Env which has version and b) it's in the server component that has the env variable VERSION set in its values.yaml.

An alternative fix could be to not move version, but give a default value as second parameter to getEnvVar

That's what I was considering as well, but in the end I decided it's safer to not have a field declared than to allow it have a bogus value.

[JanKoehnlein]

/uncc @laushinka

[JanKoehnlein]

/unhold