using core env vars instead of hardcoded #14
Conversation
jhsinger-klotho
had a problem deploying
to
integ_test
GitHub Actions
Failure
jhsinger-klotho
had a problem deploying
to
integ_test
GitHub Actions
Failure
jhsinger-klotho
had a problem deploying
to
integ_test
GitHub Actions
Failure
jhsinger-klotho
had a problem deploying
to
integ_test
GitHub Actions
Failure
jhsinger-klotho
had a problem deploying
to
integ_test
GitHub Actions
Failure
|
ran go generate locally and theres more changes, will need to include but leaving out for now |
| EnvironmentVariableDirective = "environment_variables" | ||
| EnvironmentVariableDirective = "environment_variables" | ||
| ORM_ENV_VAR_NAME_SUFFIX = "_PERSIST_ORM_CONNECTION" | ||
| REDIS_PORT_ENV_VAR_NAME_SUFFIX = "_PERSIST_REDIS_PORT" |
There was a problem hiding this comment.
DOuble checking we don't have a need to differentiate between redis cluster vs node
There was a problem hiding this comment.
#12 should ensure theres never 2 persist resources with the same id, so we should never run into conflicts.
| Kind: deployment.Kind, | ||
| Type: string(EnvironmentVariableTransformation), | ||
| Key: v, | ||
| EnvironmentVariable: envVar, |
There was a problem hiding this comment.
hm this field feels very specific to env vars, do we need to have the envVar value here? Isn't the rest of the fields the same and the key would be just be the envVar.name?
There was a problem hiding this comment.
well the reason im including all of it is because our pulumi plugin knows how to process our core.Environment struct as a dependency environment variable and thats the value we need here. The key remainder is for helm to know how to set the values in the install directory, but it doesnt know how to get an rds instances connection string, etc
| @@ -193,6 +193,13 @@ func (chart *KlothoHelmChart) handleExecutionUnit(unit *HelmExecUnit, eu *core.E | |||
| return nil, err | |||
| } | |||
| values = append(values, upstreamValues...) | |||
|
|
|||
| persistValues, err := unit.handlePersistForExecUnit(deps) | |||
There was a problem hiding this comment.
ah yeh, this seems like a better place for this 👍
| envVars = append(envVars, core.GenerateOrmConnStringEnvVar(target.Name, target.Kind)) | ||
| case string(core.PersistRedisNodeKind): | ||
| envVars = append( | ||
| envVars, |
There was a problem hiding this comment.
if we don't need to distinguish between node/cluster then we can collapse these into
case string(core.PersistRedisNodeKind):
case string(core.PersistRedisClusterKind):
... logic
There was a problem hiding this comment.
ah, good catch, will update
| Namespace: "default", | ||
| }, | ||
| deploymentYaml: `apiVersion: apps/v1 | ||
| kind: Deployment |
There was a problem hiding this comment.
side note - do you have an easy way of working w/ these charts in string form? When I was working on the tests like 80% of the pain was getting the output to correctly match w/ the spacing vs tabbing and stuff my editor was doing by default (like setting a tab when I hit enter)
There was a problem hiding this comment.
yeah you always have to use spaces, its a bit annoying, but would be really hard to test all of these cases end to end with integ tests so im just trying to get things in place here. We maybe could write some k8s testing suite to check our compiled output instead of unit tests though
|
No blocking comments |
|
• Does any part of it require special attention?
• Does it relate to or fix any issue? makes progress towards #601. It doesnt pull back all of the environment variables, but removes the need of hardcoding for redis, orm, helm env vars.
A lot of this is unit tests, but heres what isnt
python and js persist - we use the new method generateEnvVar (for orm and redis respectively) and then attach that to the exec unit.
Kubernetes - We attach the core.Environment Variable to the value so that our infra understands what it will need to inject. This prevents more hardcoding and creation if the same env vars as before without the _
DeployLib and index.tmpl - We always pass the unit.EnvironmentVariables to whichever function is creating the exec unit
Standard checks