Using `ARG --global EARTHLY_GIT_HASH` results in error when using `FROM --pass-args` about earthly

[ingwarsw]

Today I got into same issue in one of our build pipelines..

Simplest pipeline that fails..

VERSION 0.8
ARG --global EARTHLY_GIT_HASH

KBT_PREPARE_IMAGE:
    FUNCTION

kbt-prepare:
    FROM alpine
    DO --pass-args +KBT_PREPARE_IMAGE

kbt-image:
    FROM alpine
    BUILD --pass-args +kbt-prepare

[antoniomdk]

Same issue here. It seems if a target uses a built-in arg and uses --pass-args to invoke other targets, the issue is raised.

[dustyhorizon]

I wrote a simple patch to ignore variable names that matches any built-in args instead of erroring out, I can create the PR but I am not sure if this behavior is acceptable by the Earthly team.

I wasn't too sure how to propagate the offending variables up to a "console writer" or how I can refer to the console within the parseArg function. I believe this can be better polished by the core devs but this works for my issue right now.

diff --git a/variables/parse.go b/variables/parse.go
index 9355bb55..805a5f34 100644
--- a/variables/parse.go
+++ b/variables/parse.go
@@ -1,6 +1,7 @@
 package variables

 import (
+       "fmt"
        "os"
        "strings"

@@ -72,13 +73,14 @@ func parseArg(arg string, pncvf ProcessNonConstantVariableFunc, current *Collect
        }
        if hasValue {
                if reserved.IsBuiltIn(name) {
-                       return "", "", errors.Errorf("value cannot be specified for built-in build arg %s", name)
-               }
-               v, err := parseArgValue(name, value, pncvf)
-               if err != nil {
-                       return "", "", err
+                       fmt.Printf("value cannot be specified for built-in build arg %s and will be ignored\n", name)
+               } else {
+                       v, err := parseArgValue(name, value, pncvf)
+                       if err != nil {
+                               return "", "", err
+                       }
+                       return name, v, nil
                }
-               return name, v, nil
        }
        v, ok := current.Get(name, WithActive())
        if !ok {

[alexcb]

Thanks for taking a deep dive @dustyhorizon

I think it would be better to tackle it under

earthly/earthfile2llb/converter.go

Line 1708 in 34320d5

overriding = variables.CombineScopesInactive(overriding, c.varCollection.Overriding(), c.varCollection.Args(), c.varCollection.Globals())

and

earthly/earthfile2llb/converter.go

Line 1901 in 34320d5

overriding = variables.CombineScopes(overriding, c.varCollection.Overriding(), c.varCollection.Args(), c.varCollection.Globals())

However, there's some additional issues though that we need to think about, for example consider:

VERSION 0.8
FROM alpine

a:
  RUN env | grep EARTHLY

test:
  ARG EARTHLY_GIT_HASH
  ARG EARTHLY_TARGET
  ARG EARTHLY_NOT_A_RESERVED=hello
  BUILD --pass-args +a

when running +test, I would expect regular non reserved args to be passed in, e.g. the EARTHLY_NOT_A_RESERVED argument. so when we get to the a target, we should see:

                  +a | EARTHLY_NOT_A_RESERVED=hello

But I don't know if we should also see the built ins, e.g.

                  +a | EARTHLY_TARGET=+test
                  +a | EARTHLY_GIT_HASH=1d4969b125fb12b010684a1e7fd98a1008086022

I'm leaning towards yes, since pass args should pass all the args in it's scope (and it's designed to prevent users from having to always re-declare ARGs)

What I don't fully know is what we should expect the value of EARTHLY_TARGET to be -- it was defined under the test target, but it was passed to a -- should its value change to be EARTHLY_TARGET=a even though it wasn't defined under a? I think so, but am not entirely sure -- it's an edge case we didn't think about.

edit: let's not pass them along -- after combining the scopes, we could introduce a new function like overriding = RemoveReserved(overriding)

[alexcb]

In second thoughts (after chatting with @mikejholly), it seems easier to avoid these edge cases of what the values should be by simply not passing built in args.

The --pass-args feature was created to make it easier to pass args between different targets that are chained between multiple Earthfiles (without having to write tons of glue code); builtin arguments don't have this problem because they are builtin and can simply be declared in the final target and they'll be correctly populated.

[ingwarsw]

@alexcb Your last comment sees reasonable..
How hard it will be to release that change?
Cause it started to pop up in many places in our pipelines (cause I upgraded to 0.8 by default)..

[alexcb]

How hard it will be to release that change?

I moved it up in our todo priority; however we'll gladly accept a PR if anyone is able to get to it sooner.

[alexcb]

potential fix in #3872

[alexcb]

@dustyhorizon can you provide a concrete example for this issue? I went to test it against v0.8.4 and couldn't replicate the issue; here's what I tried:

VERSION 0.8

test:
  FROM alpine
  ARG EARTHLY_GIT_HASH
  RUN echo here1 && env | grep EARTHLY
  BUILD --pass-args +subtest

subtest:
  FROM alpine
  RUN echo here2 && env | grep EARTHLY

and

VERSION 0.8
ARG --global EARTHLY_GIT_HASH

test:
  FROM alpine
  RUN echo here1 && env | grep EARTHLY
  BUILD --pass-args +subtest

subtest:
  FROM alpine
  RUN echo here2 && env | grep EARTHLY

both output the same values:

/h/a/j/pass-args-builtin+test | here1
/h/a/j/pass-args-builtin+test | EARTHLY_GIT_HASH=434d11e85797c9199b62fbb85fb1e4c03fb89cb5
/h/a/j/pass-args-builtin+subtest | here2
/h/a/j/pass-args-builtin+subtest | EARTHLY_GIT_HASH=434d11e85797c9199b62fbb85fb1e4c03fb89cb5

[alexcb]

fixed in v0.8.5