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

Env variables don't work in if-else-endif templates #488

Closed
AtomToast opened this issue May 27, 2024 · 14 comments · Fixed by soraxas/yadm#1
Closed

Env variables don't work in if-else-endif templates #488

AtomToast opened this issue May 27, 2024 · 14 comments · Fixed by soraxas/yadm#1
Assignees
Labels

Comments

@AtomToast
Copy link

Describe the bug

Similarly to #486, trying to use env variables in {% if env.EXAMPLE == "example" %} with the default template processor does not work.

To reproduce

Can this be reproduced with the yadm/testbed docker image: Yes
script.gz

Steps to reproduce the behavior:

  1. export TEST=testing
cat << EOF > test.txt##template         
{% if env.TEST == "testing" %}
success
{% else %}
fail
{% endif %}
EOF
  1. yadm add test.txt##template
  2. cat test.txt

Expected behavior

The file should contain success, however instead it uses the fail branch.

Environment

  • Operating system: Arch Linux
  • Version yadm: 3.2.2
  • Version Git: 2.45.1

Additional context

From a brief look at the code, it seems that this is actually just not implemented right now?

@AtomToast AtomToast added the bug label May 27, 2024
@rasa
Copy link
Contributor

rasa commented May 27, 2024

@rasa
Copy link
Contributor

rasa commented May 27, 2024

On my system, env.HOME substitutes correctly. Ubuntu 24.04 and

$ yadm version
bash version 5.2.21(1)-release
 git version 2.43.0
yadm version 3.2.2
$ awk --version| head -n 1
GNU Awk 5.2.1, API 3.2, PMA Avon 8-g1, (GNU MPFR 4.2.1, GNU MP 6.3.0)

@AtomToast
Copy link
Author

Please correct me if I'm wrong but to me this is again only the variable substitution part of the code. This works for me.
What doesn't work for me are if blocks. As per https://yadm.io/docs/templates, in the if-else-endif section.

@rasa
Copy link
Contributor

rasa commented May 29, 2024

@AtomToast Good point. I haven't tested it, but the env. logic is the same as the other yadm. so I am really surprised it's not working. See https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L426-L431

@AtomToast
Copy link
Author

For me this is 100% reproducable.

The code you linked is part of the replaces_vars() function, which does not seem to be called in the conditions() function.
There all I can see is this https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L441 which I believe is the yadm. part of the logic? There is no EVIRON[label] call

@rasa
Copy link
Contributor

rasa commented May 29, 2024

@AtomToast You are correct. Just below
https://github.com/TheLocehiliosan/yadm/blob/0a5e7aa353621bd28a289a50c0f0d61462b18c76/yadm#L439-L444
we need to add

    for (label in ENVIRON) {
       value = ENVIRON[label] 
       pattern = sprintf("%s%s|", pattern, condition_helper(label, value)); 
    }

@AtomToast
Copy link
Author

That alone seems to not have done it for me. Is there perhaps something else missing?

Copy link

This issue has been labeled as stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Jul 29, 2024
@AtomToast
Copy link
Author

@rasa Hey, sorry for the bother but the bot marked this as stale. Have you managed to make this work?

@rasa
Copy link
Contributor

rasa commented Aug 3, 2024

@AtomToast No sorry, I haven't. Hopefully @TheLocehiliosan can un-stale this, as I can't.

@soraxas
Copy link
Contributor

soraxas commented Aug 14, 2024

@AtomToast Does these changes fixes for you?

https://github.com/soraxas/yadm/pull/1/files

diff --git a/yadm b/yadm
index bfbcd81..a61f37f 100755
--- a/yadm
+++ b/yadm
@@ -432,20 +432,23 @@ function replace_vars() {
 }
 function condition_helper(label, value) {
   gsub(/[\\.^$(){}\[\]|*+?]/, "\\\\&", value)
-  return sprintf("yadm\\.%s" blank "*==" blank "*\"%s\"", label, value)
+  return sprintf("%s" blank "*==" blank "*\"%s\"", label, value)
 }
 function conditions() {
   pattern = ifs blank "+("
+  for (label in ENVIRON) {
+    pattern = sprintf("%s%s|", pattern, condition_helper("env\\." label, ENVIRON[label]));
+  }
   for (label in c) {
     if (label != "class") {
       value = c[label]
-      pattern = sprintf("%s%s|", pattern, condition_helper(label, value));
+      pattern = sprintf("%s%s|", pattern, condition_helper("yadm\\." label, value));
     }
   }
   split(classes, cls_array, "\n")
   for (idx in cls_array) {
     value = cls_array[idx]
-    pattern = sprintf("%s%s|", pattern, condition_helper("class", value));
+    pattern = sprintf("%s%s|", pattern, condition_helper("yadm\\." "class", value));
   }
   sub(/\|$/, ")" blank "*%}$", pattern)
   return pattern

(I shall note that the current template replacement seems pretty inefficient as it needs to loop-through all class/envars everytime; though its still "fast enough" and the operation would be rare)

@AtomToast
Copy link
Author

@soraxas Unfortunately no :/
I tried it with both the test case from the original post and some additional ones. I also tried cloning your entire repo which seems to have some additional changes.

Does it work for you with those changes?

@soraxas
Copy link
Contributor

soraxas commented Aug 15, 2024

Yes it does works for me

image

Have you point your $PATH to your cloned repo (or maybe it's still using the older yadm)?

@AtomToast
Copy link
Author

I first directly edited the yadm binary in /bin. When using the cloned repo I called yadm with an absolute path.
For some reason, now having retested it on my laptop, it seems to work now. Not sure what I did wrong on my other PC or if there was some other variable

erijo added a commit to erijo/yadm that referenced this issue Oct 27, 2024
The awk script now performs all processing in the BEGIN block using an
implementation that is capable of handling nested if statements. This fixes
issue yadm-dev#436. Includes are now handled in the same way as the main file which
means that recursive includes and if statements in includes works as
expected. This fixes yadm-dev#406.

All variables are handled in the same way now so it's now possible to use env
variables in if statements. This fixes yadm-dev#488.

Also add support for != in addition to == (fixes yadm-dev#358). Thus it's now
e.g. possible to check if a variable is set (yadm-dev#477) by doing:

{% if yadm.class != ""%}
Class is set to {{ yadm.class }}
{% endif %}

Possible breaking change: An error will be issued if a non-existent yadm or env
variable is referenced in an if statement or in a variable substitution.
erijo added a commit to erijo/yadm that referenced this issue Oct 27, 2024
The awk script now performs all processing in the BEGIN block using an
implementation that is capable of handling if statements which contain nested
if statments (fixes yadm-dev#436). To make nested ifs look better, if, else and endif
lines can now have optional whitespace before {%.

Includes are now handled in the same way as the main file which means that
included files can both include other files and have if statements in addition
to variables (fixes yadm-dev#406). Include lines can now also have optional whitespace
before {%.

All variables are handled in the same way now so it's now possible to use env
variables in if statements (fixes yadm-dev#488).

Also add support for != in addition to == (fixes yadm-dev#358). Thus it's now
e.g. possible to check if a variable is set (yadm-dev#477) by doing:

{% if yadm.class != ""%}
Class is set to {{ yadm.class }}
{% endif %}

Possible breaking change: An error will be issued if a non-existent yadm or env
variable is referenced in an if statement or in a variable substitution.
erijo added a commit that referenced this issue Nov 6, 2024
* Support nested ifs in default template (#436)
* Support include and ifs in default template includes (#406)
* Support environment variables in ifs in default template (#488)
* Support != in default template (#358, #477)
erijo added a commit that referenced this issue Nov 8, 2024
 * Support nested ifs in default template (#436)
 * Support include and ifs in default template includes (#406)
 * Support environment variables in ifs in default template (#488)
 * Support != in default template (#358, #477)
 * Fix multiple classes in default template on macOS (#437)
@erijo erijo closed this as completed in 8ba9823 Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants