FreightFrom Expression functions should return nil if not found instead of erroring

[jessesuen]

Description

I'm trying to create a re-usable promo task that might be called with an image Freight, or it might be called with git Freight. And so I want to conditionally run a step depending on the type of freight we are dealing with. I tried doing this:

  - uses: yaml-update
    if: ${{ imageFrom( vars.image ) != nil }}

But I get this error:

step execution failed: error checking if step 1 should be skipped: reflect: call of reflect.Value.Call on zero Value (1:2) | imageFrom( vars.image ) != nil | .^

I believe this happens because we error if Freight is not found, instead of returning nil without error:

kargo/internal/controller/freight/finder.go

Lines 80 to 97 in 9c17446

	// We know exactly what we're after, so this should be easy
	for i := range freight {
	f := &freight[i]
	if f.Origin.Equals(desiredOrigin) {
	for j := range f.Commits {
	c := &f.Commits[j]
	if libGit.NormalizeURL(c.RepoURL) == repoURL {
	return c, nil
	}
	}
	}
	}
	// If we get to here, we looked at all the FreightReferences and didn't find
	// any that came from the desired origin. This could be because no Freight
	// from the desired origin has been promoted yet.
	return nil, NotFoundError{
	msg: fmt.Sprintf("commit from repo %s not found in referenced Freight", repoURL),
	}

When the freight is not found, we should allow for this to return nil without error, since there are use cases where we expect it not to exist.

Alternatively, we may need other helpers to determine if freight exists that doesn't error, but I think returning nil would be preferred.

Screenshots

Steps to Reproduce

  - uses: yaml-update
    if: ${{ imageFrom( vars.image ) != nil }}

Version

v1.3.1

[hiddeco]

Think the actual issue is that if (and vars) at present do not have access to functions.

[jessesuen]

Ah yes, you are right in that lack of support for functions in if and vars is what is causing the reflect: call of reflect.Value.Call on zero Value error. But based on what I see in the code, and another test I just tried, we will still hit this issue. I just tried this:

  - uses: git-clone
    config:
      repoURL: ${{ vars.repoURL }}
      checkout:
      - branch: ${{ commitFrom( vars.repoURL ) ?? "main" }}
        path: ./src

This will error with the not found in referenced Freight error:

step execution failed: step 0 met error threshold of 1: failed to get step config: commit from repo https://github.com/jessesuen/kargo-rendered-branches not found in referenced Freight (1:2) | commitFrom( vars.repoURL ) ?? "main" | .^

So even after adding access to functions in if (#3646), we don't have a way to skip steps based on whether or not a Freight of that type exists.

[krancour]

I think I agree with this change, but at the same time, I wonder if the nil pointer dereference you'll end up with instead of a very clear error won't obscure some legitimate user mistakes.

I wonder a little if some variations of the existing functions would allow you to easily do what you're trying to do without obscuring actual mistakes... something like imageOrNilFrom(...)...

But I'm really not sure of that either. 🤷‍♂