Don't exit on error when condition is not met

[olblak]

Signed-off-by: Olivier Vernin olivier@vernin.me

Don't exit on error when condition is not met

Fix #291

This pull request introduces a new final result type which is "skipped" which happen when a pipeline run is abort but not due to an error like a condition is not met.
I am modifying the final output message to easily identify those situations

Test

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

  go build -o bin/updatecli
  go test

Additionnal Information

Output

Now

- HUGO.YAML:
	Sources:
		✔ [hugoVersion] Get the latest hugo version(githubRelease)
		✔ [hugoVersionFromNetlifyConfig] Get netlify configuration(file)
		✔ [netlifyConfig] Get netlify configuration(file)
	Condition:
		✗ [docker] Test if docker Image klakegg/hugoPublished is available on docker Registry(dockerImage)
	Target:
		- [netlifyConfig]  Get netlify configuration(file)
		- [updateDockerfileArgHelmfileVersion]  Update default HUGO_VERSION value in the Dockerfile(dockerfile)



Run Summary
===========
Pipeline(s) run:
  * Changed:	  0
  * Failed:	  0
  * Skipped:	  1
  * Succeeded:   0
  * Total:	          1

Old

⚠ HUGO.YAML:
	Sources:
		✔ [hugoVersion] Get the latest hugo version(githubRelease)
		✔ [hugoVersionFromNetlifyConfig] Get netlify configuration(file)
		✔ [netlifyConfig] Get netlify configuration(file)
	Target:
		⚠ [netlifyConfig]  Get netlify configuration(file)
		⚠ [updateDockerfileArgHelmfileVersion]  Update default HUGO_VERSION value in the Dockerfile(dockerfile)



Run Summary
===========
1 job run
0 job succeed
0 job failed
1 job applied changes

[lemeurherve]

I would have used successives if instead of these else if but it's maybe just me.

[dduportal]

chaing the if means that you could allow entering different conditions (if your wrote it with an error), while the else if ensures that only one of the block is executed.

However this chain of else if should clearly use a switch / case instruction (please note that strings.Compare should also be replaced by a simple == comparison as it's targeted for a lexicographic comparison which is not really performant - https://pkg.go.dev/strings#Compare )

[lemeurherve]

In these cases, I prefer to set an "error" flag at the beginning, then successives "if" (cancelling the flag), and finally return an error if the flag is still set. Not sure how it could enter differents "if" in this actual case.
Easier to (re)read/comprehend than "if...elseif" or "switch", and (as I understood) a common pattern.

[dduportal]

Not sure how it could enter differents "if" in this actual case.
If a human messes up the instructions in the if. Happens a lot when a major refactoring or a bumpy merge conflict happen, while a "switch" would statically ensure this.

I cannot propose a suggestion here (the diff is split), but here is a proposal to give an idea:

switch report.Result {
  case "result.SUCCESS":
    successCounter++
  case "result.FAILURE":
    failedCounter++
  case "result.ATTENTION":
    changedCounter++
  case "result.SKIPPED":
    skippedCounter++
  default:
    logrus.Infof("Unknown report result %q with named %q.", report.Result, report.Name)
}

[olblak]

Thanks for the review, the switch syntax makes more sense here.
Regarding the syntax to compare strings, it appears that string1 == string2 is better than using strings.Compare

[dduportal]

Good job, it sounds good for me for the use case.

There is a chain of else if + Strings.compare that should be changed to a swicth / case and string equalities== though

[dduportal]

chaing the if means that you could allow entering different conditions (if your wrote it with an error), while the else if ensures that only one of the block is executed.

However this chain of else if should clearly use a switch / case instruction (please note that strings.Compare should also be replaced by a simple == comparison as it's targeted for a lexicographic comparison which is not really performant - https://pkg.go.dev/strings#Compare )

[dduportal]

🚀