Add support for setting env variables in a workspace #74

bdwyertech · 2023-02-06T05:58:11Z

Description of your changes

Goal of this was to pass through env vars for TFEnv. This would allow me to pin or constrain Terraform versions in my versioned XRs. Perhaps it could be used for something else, such as specifying the terraform source, or provider-specific debug flags.

I have:

Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Upbound-CLA · 2023-02-06T05:58:15Z

All committers have signed the CLA.

bobh66

Thanks for the contribution! Looks good overall, a couple of comments inline. Can you also add some documentation in the README and maybe an example in examples/ ?

internal/terraform/terraform.go

package/crds/tf.upbound.io_workspaces.yaml

bdwyertech · 2023-02-06T16:48:06Z

Thanks for the contribution! Looks good overall, a couple of comments inline. Can you also add some documentation in the README and maybe an example in examples/ ?

Sure, I'll fix the tests and get an example into the README

bobh66 · 2023-02-08T15:20:34Z

@bdwyertech I'm concerned that this is now two features in one PR - the original ENV support and the addition of tfenv. I'd prefer to keep them separate - the tfenv change is much more significant and I'd like to be able to validate it separately.

bdwyertech · 2023-02-08T19:50:29Z

OK, I can break that out separately, however this is a prereq to use tfenv.

bobh66 · 2023-02-08T20:03:15Z

Thanks - I think the env var support is fine, and could even be extended to support valueFrom and secrets at some point. That would enable credentials to be loaded into the environment from a Secret, which I know some terraform providers might need.

Can you open an issue with some more background/details for the tfenv support? It would be good to get some specifics and discussion around the topic. It's a significant change to support multiple versions of terraform. Thanks!

bdwyertech · 2023-02-08T20:38:32Z

Thanks - I think the env var support is fine, and could even be extended to support valueFrom and secrets at some point. That would enable credentials to be loaded into the environment from a Secret, which I know some terraform providers might need.

Can you open an issue with some more background/details for the tfenv support? It would be good to get some specifics and discussion around the topic. It's a significant change to support multiple versions of terraform. Thanks!

OK, I've broken tfenv into its branch and will create a subsequent PR. For now, this is MR just adds env var support.

bdwyertech · 2023-02-09T17:57:21Z

Added support for ConfigMap/Secret reference.

internal/terraform/terraform.go

nakamume · 2023-03-06T09:04:07Z

@bobh66 @bdwyertech Though not a blocker but this feature would greatly improve our workflows. I'd like continue with these changes. I guess, I'll have to fork and open a new PR. Please let me know if there's some way to contribute to this same PR.

addisonj · 2023-05-24T15:27:00Z

It looks like some the changes were split out and the requested changes were made.

Is this ready for another review? Is there anything else preventing this from making progress?

This would be super useful to me and would be happy to help contribute, but just need clear direction from the maintainers (@bobh66) on what is missing to get this over the line.

bobh66 · 2023-05-24T16:22:10Z

Is this ready for another review? Is there anything else preventing this from making progress?

There are lint errors blocking the CI pipeline

ytsarev · 2023-05-25T00:35:54Z

internal/terraform/terraform.go

 	cmd.Env = append(cmd.Env, "TF_CLI_CONFIG_FILE=./.terraformrc")
+	cmd.Env = append(cmd.Env, h.Envs...)


Suggested change

cmd.Env = append(cmd.Env, "TF_CLI_CONFIG_FILE=./.terraformrc")

cmd.Env = append(cmd.Env, h.Envs...)

cmd.Env = append(cmd.Env, "TF_CLI_CONFIG_FILE=./.terraformrc", h.Envs...)

You cant do that.

21:47:07 [ .. ] go build linux_amd64 github.com/upbound/provider-terraform/apis/v1beta1 github.com/upbound/provider-terraform/internal/terraform # github.com/upbound/provider-terraform/internal/terraform internal/terraform/terraform.go:177:65: too many arguments in call to append have ([]string, string, []string) want ([]string, ...string)

ytsarev · 2023-05-25T00:40:16Z

/test-examples="examples/workspace-inline-aws.yaml"

internal/controller/workspace/workspace.go

addisonj · 2023-05-30T20:33:32Z

@bobh66 it looks like all requested changes have been made?

rrs-celonis · 2023-07-25T15:24:18Z

Eagerly waiting for this to get merged

ytsarev · 2023-08-11T22:03:31Z

/test-examples="examples/workspace-inline-aws.yaml"

ytsarev · 2023-08-28T19:50:40Z

/test-examples="examples/workspace-inline-aws.yaml"

ytsarev · 2023-08-29T14:46:16Z

/test-examples="examples/workspace-inline-aws.yaml"

senare · 2023-10-13T14:08:05Z

Any update on this ?
What can I do t to help this move along ?

ytsarev · 2023-10-17T07:13:39Z

@senare Ideally we need to

extend the unit tests at https://github.com/upbound/provider-terraform/pull/74/files#diff-d690cfcf16b477fcc4546470aa24e791e2593cc110ff377297c2be8db1aad88c to cover the env variable control,
provide e2e example for uptest
Extend documentation as it currently does not mention the possibility of referencing Secret
Thanks in advance for your help!

(cherry picked from commit 2acfe08)

ytsarev · 2024-04-03T19:12:25Z

/test-examples="examples/workspace-inline-aws.yaml"

Signed-off-by: Yury Tsarev <yury@upbound.io>

ytsarev

I've rebased this PR with the recent state of the main branch and resolved some conflicts
Added e2e example to examples/environment to test environment propagation
Performed exploratory e2e test, everything works as expected

k get workspace
NAME                     SYNCED   READY   AGE
sample-inline-with-env   True     True    19m

The environment data was properly propagated to the AWS tags as expected

Everything looks good to me.

@bobh66 As I've made additions to this PR could you please make another review from your side before we merge it? Thanks!

@bdwyertech thank you so much for this great contribution!

bobh66

A couple of minor comments and one question, but otherwise it LGTM.

bobh66 · 2024-04-03T20:11:36Z

README.md

+    # Environment variables can be passed through
+    env:
+      - name: TFENV_TERRAFORM_VERSION
+        value: '1.3.5'


Will this be confusing since you can't really change the version of terraform that we run?

I've renamed the env var to a more neutral version

README.md

bobh66 · 2024-04-03T21:34:22Z

examples/environment/workspace-inline-env-aws.yaml

+        description = "Environment Value From Secret"
+        type        = string
+      }
+    env:


nit: I think it flows better to have the env definition before the module definition, but it's not critical to change

Agree, example amended

bobh66 · 2024-04-03T21:37:31Z

internal/terraform/terraform.go

@@ -185,6 +188,7 @@ func (h Harness) Init(ctx context.Context, o ...InitOption) error {
 		cmd.Env = append(cmd.Env, e)
 	}
 	cmd.Env = append(cmd.Env, "TF_CLI_CONFIG_FILE=./.terraformrc")
+	cmd.Env = append(cmd.Env, h.Envs...)


This line appends h.Envs... unconditionally, where all of the other usages check the length and only append if the length is non-zero. Is there a reason to check the length in the other places and not check it here? Or does it not need to be checked at all?

Good catch, it is probably harmless from the way how append behaves, but let's make it cleaner, I've added the conditional

So just for solidarity, the reason I did not add the check here was because the line above already appended. My reasoning behind checking in the other cases was because I'd rather inherit the default behavior of cmd.Env. If you notice, I pick up os.Environ() which is the default if you do not set cmd.Env. Should the Go upstream behavior change or whatever, my goal was to avoid interfering... if that makes sense. That said, there is no harm here in adding the check, probably some picosecond of compute or something.

Signed-off-by: Yury Tsarev <yury@upbound.io>

ytsarev · 2024-04-03T22:17:03Z

@bobh66 thanks a lot for the review. I've addressed the PR feedback. Please take another look

bobh66

Looks good - thanks @bdwyertech and @ytsarev !

bdwyertech · 2024-04-03T23:33:54Z

Better late than never... Thanks guys, I can finally stop using my fork :-)

jeanduplessis added the community label Feb 6, 2023

bobh66 requested changes Feb 6, 2023

View reviewed changes

internal/terraform/terraform.go Show resolved Hide resolved

package/crds/tf.upbound.io_workspaces.yaml Outdated Show resolved Hide resolved

package/crds/tf.upbound.io_workspaces.yaml Outdated Show resolved Hide resolved

bdwyertech force-pushed the env-support branch from c4e7c6a to 0d96b42 Compare February 8, 2023 20:30

bdwyertech force-pushed the env-support branch from 0d96b42 to b01aecc Compare February 9, 2023 17:54

bdwyertech force-pushed the env-support branch 2 times, most recently from 6e33ddd to fb24ab0 Compare February 9, 2023 19:20

bobh66 reviewed Feb 17, 2023

View reviewed changes

internal/terraform/terraform.go Show resolved Hide resolved

bobh66 mentioned this pull request Mar 1, 2023

Allow use of env variables for providerconfig #96

Open

bdwyertech force-pushed the env-support branch from fb24ab0 to fd44e10 Compare May 10, 2023 20:49

bdwyertech requested review from ytsarev, negz, ulucinar and sergenyalcin as code owners May 10, 2023 20:49

bdwyertech force-pushed the env-support branch from fd44e10 to 212996a Compare May 10, 2023 21:04

bdwyertech force-pushed the env-support branch from 212996a to bad2a8d Compare May 24, 2023 23:09

ytsarev reviewed May 25, 2023

View reviewed changes

bdwyertech force-pushed the env-support branch 2 times, most recently from 7778b57 to a739694 Compare May 25, 2023 01:48

bobh66 reviewed May 25, 2023

View reviewed changes

internal/controller/workspace/workspace.go Outdated Show resolved Hide resolved

bdwyertech force-pushed the env-support branch from 5a5c9fe to ee96612 Compare May 25, 2023 05:27

ytsarev force-pushed the env-support branch from ee96612 to 7e64739 Compare August 11, 2023 22:01

ytsarev requested a review from turkenf as a code owner August 11, 2023 22:01

ytsarev force-pushed the env-support branch from 7e64739 to d798cd1 Compare August 29, 2023 14:43

senare mentioned this pull request Oct 19, 2023

Need some help getting started #200

Open

bdwyertech and others added 3 commits April 3, 2024 21:00

Add support for setting env variables in a workspace

92bc309

(cherry picked from commit 2acfe08)

Update mocks & README

4fdad62

prealloc

1c35275

ytsarev force-pushed the env-support branch from d798cd1 to 1c35275 Compare April 3, 2024 19:06

ytsarev added 2 commits April 3, 2024 21:27

Add new up

8cfe35b

Signed-off-by: Yury Tsarev <yury@upbound.io>

Add e2e example for environment usage and extend README

3f88312

Signed-off-by: Yury Tsarev <yury@upbound.io>

ytsarev approved these changes Apr 3, 2024

View reviewed changes

bobh66 reviewed Apr 3, 2024

View reviewed changes

Address PR feedback

7622434

Signed-off-by: Yury Tsarev <yury@upbound.io>

ytsarev requested a review from bobh66 April 3, 2024 22:16

bobh66 approved these changes Apr 3, 2024

View reviewed changes

ytsarev merged commit 51dda57 into upbound:main Apr 3, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for setting env variables in a workspace #74

Add support for setting env variables in a workspace #74

bdwyertech commented Feb 6, 2023 •

edited

Loading

Upbound-CLA commented Feb 6, 2023 •

edited

Loading

bobh66 left a comment

bdwyertech commented Feb 6, 2023

bobh66 commented Feb 8, 2023

bdwyertech commented Feb 8, 2023

bobh66 commented Feb 8, 2023

bdwyertech commented Feb 8, 2023

bdwyertech commented Feb 9, 2023

nakamume commented Mar 6, 2023

addisonj commented May 24, 2023

bobh66 commented May 24, 2023

ytsarev May 25, 2023

bdwyertech May 25, 2023

ytsarev commented May 25, 2023

addisonj commented May 30, 2023

rrs-celonis commented Jul 25, 2023

ytsarev commented Aug 11, 2023

ytsarev commented Aug 28, 2023

ytsarev commented Aug 29, 2023

senare commented Oct 13, 2023

ytsarev commented Oct 17, 2023 •

edited

Loading

ytsarev commented Apr 3, 2024

ytsarev left a comment

bobh66 left a comment

bobh66 Apr 3, 2024

ytsarev Apr 3, 2024 •

edited

Loading

bobh66 Apr 3, 2024

ytsarev Apr 3, 2024

bobh66 Apr 3, 2024

ytsarev Apr 3, 2024

bdwyertech Apr 3, 2024

ytsarev commented Apr 3, 2024

bobh66 left a comment

bdwyertech commented Apr 3, 2024

		cmd.Env = append(cmd.Env, "TF_CLI_CONFIG_FILE=./.terraformrc")
		cmd.Env = append(cmd.Env, h.Envs...)

Add support for setting env variables in a workspace #74

Add support for setting env variables in a workspace #74

Conversation

bdwyertech commented Feb 6, 2023 • edited Loading

Description of your changes

How has this code been tested

Upbound-CLA commented Feb 6, 2023 • edited Loading

bobh66 left a comment

Choose a reason for hiding this comment

bdwyertech commented Feb 6, 2023

bobh66 commented Feb 8, 2023

bdwyertech commented Feb 8, 2023

bobh66 commented Feb 8, 2023

bdwyertech commented Feb 8, 2023

bdwyertech commented Feb 9, 2023

nakamume commented Mar 6, 2023

addisonj commented May 24, 2023

bobh66 commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytsarev commented May 25, 2023

addisonj commented May 30, 2023

rrs-celonis commented Jul 25, 2023

ytsarev commented Aug 11, 2023

ytsarev commented Aug 28, 2023

ytsarev commented Aug 29, 2023

senare commented Oct 13, 2023

ytsarev commented Oct 17, 2023 • edited Loading

ytsarev commented Apr 3, 2024

ytsarev left a comment

Choose a reason for hiding this comment

bobh66 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytsarev Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytsarev commented Apr 3, 2024

bobh66 left a comment

Choose a reason for hiding this comment

bdwyertech commented Apr 3, 2024

bdwyertech commented Feb 6, 2023 •

edited

Loading

Upbound-CLA commented Feb 6, 2023 •

edited

Loading

ytsarev commented Oct 17, 2023 •

edited

Loading

ytsarev Apr 3, 2024 •

edited

Loading