Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix scenario where env vars are empty #85

Merged
2 commits merged into from
Jan 13, 2023
Merged

fix scenario where env vars are empty #85

2 commits merged into from
Jan 13, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jan 13, 2023

When the env var was empty, the end of the method call was , ) instead of ,[]).

Standard checks

  • Unit tests: not unit tested
  • Docs: n/a
  • Backwards compatibility: no issues

@@ -112,7 +112,7 @@ export = async () => {
{{range $unit := .ExecUnits -}}

{{- if eq $unit.Type "fargate"}}
cloudLib.createEcsService("{{$unit.Name}}",{{jsonPretty $unit.Params | indent 4}} as Partial<awsx.ecs.Container>, {{- if $unit.EnvironmentVariables}} , {{jsonPretty $unit.EnvironmentVariables | indent 4}} {{end}});
cloudLib.createEcsService("{{$unit.Name}}",{{jsonPretty $unit.Params | indent 4}} as Partial<awsx.ecs.Container>, {{- if $unit.EnvironmentVariables}} {{jsonPretty $unit.EnvironmentVariables | indent 4}} {{else}} [] {{end}});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the library can handle the undefined fine:

    generateExecUnitEnvVars(
        execUnitName: string,
        env_vars?: any[]
    )

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so you're saying, just rm the if-else altogether? let me check if that works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying the previous version should be fine since the , is also included in the original if statement.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, it did! nice catch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm saying the previous version should be fine since the , is also included in the original if statement.

Ah, well that's not fine. The problem wasn't the lack of ,, it was that there was nothing after the comma, so the method invocation only had two args, while it expected 3. The problem wasn't the generateExecUnitEnvVars invocation, it was the createEcsService invocation.

@github-actions
Copy link

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 2%
github.com/klothoplatform/klotho/pkg/annotation 24%
github.com/klothoplatform/klotho/pkg/core 20%
github.com/klothoplatform/klotho/pkg/env_var 82%
github.com/klothoplatform/klotho/pkg/exec_unit 45%
github.com/klothoplatform/klotho/pkg/infra/kubernetes 58%
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm 52%
github.com/klothoplatform/klotho/pkg/input 63%
github.com/klothoplatform/klotho/pkg/lang 37%
github.com/klothoplatform/klotho/pkg/lang/dockerfile 0%
github.com/klothoplatform/klotho/pkg/lang/golang 9%
github.com/klothoplatform/klotho/pkg/lang/javascript 47%
github.com/klothoplatform/klotho/pkg/lang/python 60%
github.com/klothoplatform/klotho/pkg/lang/yaml 0%
github.com/klothoplatform/klotho/pkg/logging 7%
github.com/klothoplatform/klotho/pkg/multierr 95%
github.com/klothoplatform/klotho/pkg/provider/aws 60%
github.com/klothoplatform/klotho/pkg/runtime 75%
github.com/klothoplatform/klotho/pkg/static_unit 32%
github.com/klothoplatform/klotho/pkg/validation 73%
Summary 42% (3592 / 8586)

@ghost ghost merged commit 006463a into main Jan 13, 2023
@ghost ghost deleted the indexts-fix branch January 13, 2023 16:11
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants