fix: skip sources when their parent source failed

[dduportal]

Fix #285

When a source, referenced through a template function {{ source `<source id>` }} on another source, is failing,
then the "children" sources are now skipped with an error message.

No worry for conditions: if any source fail, then the condition are never run.

Test

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

$ cat .tmp/updatecli.yaml
sources:
  parentSource:
    kind: shell
    spec:
      command: ls /titi
  childSource:
    kind: shell
    depends_on:
      - parentSource
    spec:
      command: echo CHILD-{{ source "parentSource" }}
target:
  printResult:
    kind: shell
    sourceID: childSource
    spec:
      command: echo

$ go build -o ./dist/updatecli && ./dist/updatecli --debug diff -c .tmp/updatecli.yaml

example of output:

SOURCES
=======

parentSource
------------
The shell 🐚 command "ls /titi" exited on error (exit code 1) with the following output:
----
----

command stderr output was:
----
ls: /titi: No such file or directory

----
ERROR: ✗ Shell command exited on error.
Pipeline "UPDATECLI.YAML" failed
Skipping due to:
        "sources stage:\t\"template: cfg:25:35: executing \\\"cfg\\\" at <source \\\"parentSource\\\">: error calling source: Parent source \\\"parentSource\\\" failed\"\n"
sources stage:  "template: cfg:25:35: executing \"cfg\" at <source \"parentSource\">: error calling source: Parent source \"parentSource\" failed"

=============================

REPORTS:


✗ UPDATECLI.YAML:
        Sources:
                - [childSource] (shell)
                ✗ [parentSource] (shell)
        Target:

Additional Information

Tradeoff

Potential improvement

[olblak]

I think you are solving half of the problem here. I see two scenarios that we need to cover

1. Source B that depends on Source A with templating involved which is solved by this PR 🎉
2. Source B that depends on Source A without templating.

In the second scenario, I think before executing a source, we need to validate that sources listed in the dependency list succeeded.
Just before this line

Something like

for _, dependedSource := range s.dependsOn {
    if p.Sources[dependencySource].Result == result.Failure {
    // skipping source as depended source already failed
   }
}

[olblak]

I don't think that the pull request in the current stage, fully solve the problem

[dduportal]

I think you are solving half of the problem here. I see two scenarios that we need to cover

1. Source B that depends on Source A with templating involved which is solved by this PR 🎉

2. Source B that depends on Source A without templating.

In the second scenario, I think before executing a source, we need to validate that sources listed in the dependency list succeeded. Just before this line

Something like

for _, dependedSource := range s.dependsOn {
    if p.Sources[dependencySource].Result == result.Failure {
    // skipping source as depended source already failed
   }
}

Good catch, thanks! Implemented (same idea with the loop, sligthly different if cases) WDYT?

[dduportal]

Let me rebase @olblak (in the next 5 min)

[dduportal]

@olblak ready to review :)

[olblak]

What do you think to break early if we identify that a "source" failed.

Suggested change

	shouldRunSource = false
	shouldRunSource = false
	break

[olblak]

I am wondering if we need to explicitly set the current source result to result.Failure if one of the dependency failed.
Such as

Suggested change

	shouldRunSource = false
	shouldRunSource = false
	source.Result = result.FAILURE

[dduportal]

I am wondering if we need to explicitly set the current source result to result.Failure if one of the dependency failed.

That feels weird since the source was not run (e.g. skipped).
Do not forget that, as for today, the default state of a source is "skipped", which is the reason why no state change is done here (otherwise I would have set it to "skipped" instead of whatever default, because it's not run).
I vote for reporting "skipped, but I don't mind enforcing the "skipped" status here (even if it's the default), so a change of the default would not break it. WDYT ?

[dduportal]

What do you think to break early if we identify that a "source" failed.

Could be discussed, but not in the scope of this PR, because it would be a breaking change in the UX behavior. Do you mind raising an issue on that topicas enhancement?

[olblak]

Thanks for the pull request, I made minor suggestions that I don't consider blocker for merging.
Feel free to proceed

[dduportal]

@olblak thanks for the review. I've answered to your 2 elements: both are outside of the scope of the PR but are legit questions (and worth an issue).

I'm merging (as it's expected to be minor) since I got your approval, don't hesitate to open issues on the topic, or revert the PR if you see something we missed :)