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

Weird interpolation with environment: script not working #120

Closed
pmiddend opened this issue Dec 15, 2023 · 19 comments
Closed

Weird interpolation with environment: script not working #120

pmiddend opened this issue Dec 15, 2023 · 19 comments
Labels
done Done, awaiting release enhancement New feature or request

Comments

@pmiddend
Copy link

pmiddend commented Dec 15, 2023

I have the following yml file which I feed into process-compose up:

version: "0.5"

environment:
  - "I_AM_GLOBAL_EV=5"

processes:
  process1:
    description: This process will sleep for 2 seconds
    command: "echo $I_AM_GLOBAL_EV > /tmp/output"

This leads to a /tmp/output file being generated - but it's empty. Why is that? If I execute env and redirect it, I see the variable.

@F1bonacc1
Copy link
Owner

Hi @pmiddend

This is the expected behavior. What happens:

  1. The env variables engine looks for $I_AM_GLOBAL_EV env variable and replaces $I_AM_GLOBAL_EV with its value.
  2. The env variable is not found either in the environment or in .env file.
  3. $I_AM_GLOBAL_EV is converted to an empty string.
  4. The I_AM_GLOBAL_EV=5 is fed into the echo process where it is not used because of 3.

To achieve the functionality that you are after I would recommend using either .env file or vars.

Using .env file

I_AM_GLOBAL_EV=5

Using vars

version: "0.5"

vars:
  I_AM_GLOBAL_EV: 5

processes:
  process1:
    description: This process will sleep for 2 seconds
    command: "echo {{ .I_AM_GLOBAL_EV }} > /tmp/output"

P.S.
Before answering you I tested the configuration and found a small bug. It is fixed now in v0.77.6. If you'll go with the vars direction, better use the new version.

@pmiddend
Copy link
Author

What a thorough explanation, thank you very much!

@teto
Copy link

teto commented Mar 8, 2024

I dont get it, what does global environment do then ?

The env variables engine looks for $I_AM_GLOBAL_EV env variable and replaces $I_AM_GLOBAL_EV with its value.

why does it do that instead of just running the command with environment variables injected in the process ?

Referring to your example in 2) why doesn't process-compose find the
https://f1bonacc1.github.io/process-compose/configuration/

@r7vme
Copy link

r7vme commented Apr 14, 2024

Thanks for the tool, I find it really useful!

However it is not clear from the explanation (from @F1bonacc1 ) above what is the purpose of environment then. I expect example from the description to produce 5 in /tmp/output. Similar to tmuxp tool . This is really useful feature for many applications.

Thanks, Roman

@adrian-gierakowski
Copy link
Contributor

Thanks for the tool, I find it really useful!

However it is not clear from the explanation (from @F1bonacc1 ) above what is the purpose of environment then. I expect example from the description to produce 5 in /tmp/output. Similar to tmuxp tool . This is really useful feature for many applications.

Thanks, Roman

Stuff you set in environment is present in env during runtime. You can test it by calling env in you script, or by putting your script into an executable file instead of inlining it. The confusing part is the PC does $ variable substitution within the config file at load time so the vars in your inline script are replaced with their values from env (or empty if they are not set) before the script is even ran. The worst part is that there is no way to escape it, which handicaps the usability of inline scripts in PC config files. Hence I never do that and always put scripts in their own files.

@cdmistman
Copy link
Contributor

The worst part is that there is no way to escape it, which handicaps the usability of inline scripts in PC config files.

i was bit by this badly yesterday, trying to figure out why a script that i was writing like fswatch ... | while read file; do echo "found file $file"; done wasn't printing anything with pc, but was with inline commands and scripts. thanks for the explanation!

since fixing the templating would be a breaking change (major even?) it would be beneficial to mention this shortcoming in the docs to save others pain. i can open a PR later if nobody beats me to it

@F1bonacc1
Copy link
Owner

Will it help if I add the ability to escape env variables with $$?

foo:
  command: "echo log is $$ENV_TEST"
  environment:
    - 'ENV_TEST=ready'

Output: log is ready

@thenonameguy
Copy link
Contributor

I think adding a global .yml boolean disabling the interpolation would be a better start.
Your suggestion @F1bonacc1 still causes a surprise to the user, until they discover the workaround. It would still go agains the https://en.wikipedia.org/wiki/Principle_of_least_astonishment

@F1bonacc1
Copy link
Owner

F1bonacc1 commented May 12, 2024

There are a few disadvantages to the global boolean approach:

  1. Same amount of work to discover the workaround as with $$.
  2. All-or-nothing approach - either you have interpolation or not for all the processes, all the fields.
  3. Reading the boolean, will result in double parsing - not a huge issue.
  4. I believe that the level of "astonishment" won't be reduced 😄

@cdmistman
Copy link
Contributor

Will it help if I add the ability to escape env variables with $$?

that would help, yes - "add a $ to escape $ substitution" is pretty consistent though it does have the unfortunate side effect of $$$$

@adrian-gierakowski
Copy link
Contributor

Will it help if I add the ability to escape env variables with $$?

that would help, yes - "add a $ to escape $ substitution" is pretty consistent though it does have the unfortunate side effect of $$$$

That’s what Google cloudbuild does for their yaml manifests

I’d also love an option to disable env substitution globally which could be done via cli flag to avoid double parsing.

@adrian-gierakowski
Copy link
Contributor

I’d also love an option to disable env substitution globally which could be done via cli flag to avoid double parsing.

If it was up to me, I’d disable substitution by default and have it enabled with cli flag. Given how many people get tripped up on this I’d say having it opt in would be better since it someone opting into it would first need to read about it and (hopefully) understand how it works.

This would require a major version bump, but so would adding the escaping mechanism so maybe it’s a good opportunity to do both at once.

@thenonameguy
Copy link
Contributor

I’d also love an option to disable env substitution globally which could be done via cli flag to avoid double parsing.

If it was up to me, I’d disable substitution by default and have it enabled with cli flag. Given how many people get tripped up on this I’d say having it opt in would be better since it someone opting into it would first need to read about it and (hopefully) understand how it works.

This would require a major version bump, but so would adding the escaping mechanism so maybe it’s a good opportunity to do both at once.

Agreed! Hence my suggestion to add the option to disable interpolation via the yaml file first (opt out, no breakage, backward compatible). I would then update devenv process-compose YAML generation to include it.

This would immediately fix the surprise of the command: not behaving the same way as entering the same command in the given shell (bash or Powershell) for the majority of the users. In my own cases, this would eliminate the need to generate wrapper shell scripts in several processes.

Then after a while the default behavior can be changed in a major release.
This is the slower/safer strategy, that allows people to migrate over. The primary concern is that many devenv users use the unstable channel of nixpkgs, so doing a breaking change will not allow dependents of process-compose to prepare for the new behavior.

@cdmistman
Copy link
Contributor

cdmistman commented May 15, 2024

i mean fwiw, the process-compose.yaml file is versioned, so all that's needed is a config file version bump. backwards compatibility is already solved here, the default behavior can be different in config file version 0.6

that said, my 2c is that templating (imho) is indeed unnecessary, and probably better served with an opt-in. ideally users handle env vars directly in their process-compose configs, where templating is already available to them in shell utilities

@F1bonacc1
Copy link
Owner

I am currently leaning toward what @adrian-gierakowski initially suggested.

  1. Add the ability to disable substitution globally with a CLI flag.
  2. Add the proposed escaping with $$. I don't think it breaks backward compatibility since it would have been substituted to something not legible.
  3. I will properly document this.

It probably won't make everyone happy and some users will still be surprised in the future. Still, at least now there will be a well-documented explanation and a "tolerable" quick solution.

I also like the v0.6 proposal by @cdmistman I am just worried that there will be another good reason to bump it to v0.7 which will also bring this change that might break a lot of compose.yamls.

@cdmistman
Copy link
Contributor

Add the ability to disable substitution globally with a CLI flag.

perhaps an option within the yaml file as well? i'd imagine frustration when users sharing a config are shocked when they have 2 different behaviors...

@F1bonacc1
Copy link
Owner

That's a very good point @cdmistman...

I wanted to avoid the double parsing, but I guess it's a small price to pay for improved usability (sharing configuration).
New plan:

  1. Global boolean flag in process-compose.yaml.
  2. Escaping with $$.
  3. Proper documentation.

Thoughts?

@F1bonacc1 F1bonacc1 reopened this May 16, 2024
@F1bonacc1 F1bonacc1 added enhancement New feature or request done Done, awaiting release labels May 17, 2024
@F1bonacc1
Copy link
Owner

Fixed in v1.6.1

@chrissound
Copy link

I find this to be very confusing behavior. I think it would help if the docs highlighted this beahaviour. When I looked at the "variables" section in docs I thought oh vars: yeah that isn't what i'm using so I disregarded it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done Done, awaiting release enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants