feat: Allow to set environment variable within shell resources

[olblak]

Signed-off-by: Olblak me@olblak.com

Allow to set environment variable within shell resources

Fix #749

Because target shell resources, set the environment variable DRY_RUN, all other environment variable are hidden with the shell script.

Note that by default all environment variables are available for source and condition shell resource.

This affects multiple project relying on Github Action that update the PATH env variable with cached tool

Test

To test this pull request, you can run the following commands:

cp <to_package_directory>
go test

Additional Information

Tradeoff

Potential improvement

[olblak]

Manifest example

name: Shell example using environment variable

sources:
  default:
    name: Test source shell
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh

  default2:
    name: Test source shell
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh
      environments:
        - name: PATH
          inherit: true

conditions:
  default:
    name: Test source shell
    disablesourceinput: true
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh

  default2:
    name: Test source shell
    kind: shell
    disablesourceinput: true
    spec:
      command: ./examples/scripts.d/env.sh
      environments:
        - name: PATH
          inherit: true

targets:
  default:
    name: Test source shell
    disablesourceinput: true
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh

  default2:
    name: Test source shell
    kind: shell
    disablesourceinput: true
    spec:
      command: ./examples/scripts.d/env.sh
      environments:
        - name: PATH
          inherit: true

[olblak]

And the script example

#!/bin/sh 

echo "PATH: $PATH"
echo "DRY_RUN: $DRY_RUN"

[jlevesy]

👀

Most of my comments are style nits so feel free to ignore them ^^.

Let me know this this helps or not!

[olblak]

Most of my comments are style nits so feel free to ignore them ^^.

Thanks, I am always open for some learning :)

[olblak]

I now realize that I didn't address a weird behavior:

• By default source and condition inehrit from all environment variable from the updatecli process
• By default target has almost no variable ["PWD","DRY_RUN","SHLVL"]

@dduportal Do you have any opinions on which behavior should be the default for all stages

[dduportal]

I now realize that I didn't address a weird behavior:

* By default `source` and `condition` inehrit from all environment variable from the updatecli process

* By default `target` has almost no variable ["PWD","DRY_RUN","SHLVL"]

@dduportal Do you have any opinions on which behavior should be the default for all stages

I can't remember why I differentiate these behaviors. I fear that I might have been lazy for the target.

Is it ok to keep the behavior that you introduced in this PR also for target (which adding the DRY_RUN variable in the list of env to be passed, overriding the value with a warning if it already exists)

[dduportal]

Manifest example

name: Shell example using environment variable

sources:
  default:
    name: Test source shell
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh

  default2:
    name: Test source shell
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh
      environments:
        - name: PATH
          inherit: true

conditions:
  default:
    name: Test source shell
    disablesourceinput: true
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh

  default2:
    name: Test source shell
    kind: shell
    disablesourceinput: true
    spec:
      command: ./examples/scripts.d/env.sh
      environments:
        - name: PATH
          inherit: true

targets:
  default:
    name: Test source shell
    disablesourceinput: true
    kind: shell
    spec:
      command: ./examples/scripts.d/env.sh

  default2:
    name: Test source shell
    kind: shell
    disablesourceinput: true
    spec:
      command: ./examples/scripts.d/env.sh
      environments:
        - name: PATH
          inherit: true

Looks good!

WDYT to use the same configuration behavior as Docker?

environments:
  - name: PATH # Only name, eg. 'if value == ""' means "inherit"
  - name: GITHUB_TOKEN
     value: "SuperSecretTokenExposed" # Custom value that could be templatized from within updatecli

Also, I wonder if the attribute name: could not be key: or variable: ? (but nitpicking for this one)

?

[olblak]

I can't remember why I differentiate these behaviors. I fear that I might have been lazy for the target.
Is it ok to keep the behavior that you introduced in this PR also for target (which adding the DRY_RUN variable in the list of env to be passed, overriding the value with a warning if it already exists)

I guess you mean the source/condition :D I agree it doesn't make sense to us the DRY_RUN env variable for source and condition but if we don't do it then by default then the default behavior in the "os" golang package is to inherit all environment variable from the updatecli process. So I am gonna add "DRY_RUN" to source and condition as well

Also, I wonder if the attribute name: could not be key: or variable: ? (but nitpicking for this one)

TBH I hesitated implementing the inherit key, and I agree with you it doesn't really add value, so I am gonna remove it
Regarding variable key/variable instead of name/value, It's a personal taste but I prefer the name/value so unless addition input, I am gonna keep it :D

[lemeurherve]

Nit only :)

TBH I hesitated implementing the inherit key, and I agree with you it doesn't really add value, so I am gonna remove it

🎉

[olblak]

After the final evaluation, I would like to trigger a new release, as this PR is blocking several of my pipelines

[olblak]

Thanks everybody for the constructive reviews

[jetersen]

oh damn, could really use a release of this 😅

Thought this was just supported.

This is massively messing with variables in pwsh cannot even access $ENV:TEMP cannot access $ENV:PATH either

[jetersen]

Workaround :( jenkinsci/bom@b359dd5 (#1290)

[lemeurherve]

@jetersen FTR, it's now available in the v0.28.0 release

[jetersen]

Thanks for reminding me that I need to setup releases watch on this repo.