dockerimage condition fails when docker image does not exist

[treezio]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

After upgrading updatecli from v0.64.1 to v0.65.0 the condition for dockerimage fails instead of being skipped, leading to a failure output (exit 1).

Expected Behavior

The pipeline is skipped as the image does not exists meaning the condition is not met.

Steps To Reproduce

On github actions use latest action version v2.47.0 which runs on updatecli v0.65.0

Set a condition for a docker image tag that does not exist.

Scenario: The latest tag/release in the source repository is v1.47.2 . There is no v1.47.2 tag for the Docker Image in DockerHub

sources:
  latestVersion:
    kind: githubrelease
    name: Get the latest stable version
    spec:
      owner: "treezio"
      repository: "test-releases-notifications"
      token: "{{ requiredEnv .github.token }}"
      username: "{{ .github.username }}"
      versionfilter:
        kind: semver

conditions:
  IsDockerImagePublished:
    name: Check application {{ source "latestVersion" }} docker image tag is released
    kind: dockerimage
    spec:
      image: "treezio/test-notification-releases"
      architecture: amd64

The output:

SOURCES
=======

latestVersion
-------------
Searching for version matching pattern "*"
✔ GitHub release version "v1.47.2" found matching pattern "*" of kind "semver"


CHANGELOG:
----------

Release published on the 2023-10-25 16:04:57 +0000 UTC at the url https://github.com/treezio/test-releases-notifications/releases/tag/v1.47.2




CONDITIONS:
===========

IsDockerImagePublished
----------------------
ERROR: GET https://index.docker.io/v2/treezio/test-notification-releases/manifests/v1.[47](https://github.com/sequra/jenkins-test/actions/runs/6655061707/job/18084462981#step:4:48).2: MANIFEST_UNKNOWN: manifest unknown; unknown tag=v1.47.2

✗ condition not met, skipping pipeline

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

REPORTS:



✗ Bump app-update image version:
	Source:
		✔ [latestVersion] Get the latest stable app-update version
	Condition:
		✗ [IsDockerImagePublished] Check app-update v1.47.2 docker image tag is released
	Target:
		- [imageTag] 


Run Summary
===========
Pipeline(s) run:
  * Changed:	0
  * Failed:	1
  * Skipped:	0
  * Succeeded:	0
  * Total:	1
ERROR: ✗ 1 over 1 pipeline failed
ERROR: command failed: 1 over 1 pipeline failed
Error: Process completed with exit code 1.

Environment

- Location: GitHub Runner
- OS: ubuntu-22.04
- updatecli: v0.65.0

Anything else?

No response

[olblak]

Thanks for opening this issue, indeed you are right to highlight a scenario I forgot about.

The pipeline is skipped as the image does not exists meaning the condition is not met.,

The purpose of the fix introduced in 0.65.0 was to detect broken pipeline if the condition failed.

I now realize that "skipped" is different than "failed" and we need both.
So we probably need an additional setting on condition

conditions:
  IsDockerImagePublished:
    name: Check application {{ source "latestVersion" }} docker image tag is released
    kind: dockerimage
    skiponfail: true
    spec:
      image: "treezio/test-notification-releases"
      architecture: amd64

https://github.com/updatecli/updatecli/blob/5758ea9e3125d4bc9ad62f22c7d408ac49b434a0/pkg/core/pipeline/conditions.go#L46C12-L46C12

Yes skiponfail is not perfect but it's the first name that come out of my mind now
I am also wondering if we should skip on fail by default or report an error

[dduportal]

was to detect broken pipeline if the condition failed

@olblak that is strange indeed: what is a "failed" condition? It looks like the problem resides in the dockerimage plugin which should not fail but answer "false" when the condition does not find an image.

Having the image not present is not a failure at all (for a condition), otherwise it breaks a LOT of cases for condition right?
Or did I miss something?

[treezio]

Thanks for taking care guys!

I do not have much context around updatecli code, but It might be necessary to think about what means failure/error in order to implement an error/exception control.

For Instance: Specifiying a non existing docker repository could mean failure/error

[olblak]

The problem that I tried to fix in 0.65.0 has nothing to do with the dockerimage code but how we understand a pipeline result as a whole.

Let's take the following scenario

1. Fetch the latest release version of my application X
2. Check if a docker image tag is available for the version retrieved in step 1
3. Update my target if a docker image tag exist

We can understand it in two different ways

Scenario 1

The condition in step 2 is wrong because:

My target should have been update but for some reason it was not
I want Updatecli to return a RC 1 so I know that by looking at the CI job I need to investigate why something went wrong

Many times in the past I detected problems in upstream project but It's getting harder for project fully automated as I don't necessary watch the result of each pipeline.

Scenario 2

The condition step 2 is not passing because:

My target should have been update but for some reason the condition wasn't yet passing so we need to wait a little until let say the release pipeline is over.

In this case, I wouldn't consider the pipeline as failed but just the target was skipped because all conditions were not passing.

Both scenarios are valid to me prior 0.65.0, we consider all pipeline as the scenario 2, and since 0.65.0 all pipeline are considered as scenario 1

After additional thought, I think the default should be the scenario 2 but I am still interested to fail some pipeline if the condition is not passing.

I am open for the suggestion here, and considering that I'll be very busy in the two coming weeks, I am fine reverting the change introduced in 0.65.0

[treezio]

thanks for the detailed explanation!
I guess most people uses the latest v2 for github actions updatecli action, meaning they would be affected by v0.65.0. I do not know your criteria on release management as I'm a new user for updatecli, but maybe releasing a revert is just good enough until this is properly fixed in incoming iterations

[olblak]

You are right, I'll release 0.65.1 within the day

[olblak]

0.65.1 is released with the correct revert
So everything should be back to the previous situation.

I'll reopen the issue #1581 to track some additional effort is needed

Feel free to close the current issue if things are back to normal for you

[dduportal]

Thanks folks!

Food for thought:

• The condition (which skips while keeping an exit code of 0) is a really valuable feature of updatecli and should stay the default for the 0.x.y line. But honestly, no problem if we start a 1.x line and change the default if it fits the needs of the project: it's only a matter of planning and changelog well-written along the versioning scheme policy.

• However, I'm wondering if the case "source (GH release) -> condition (dockerimage) -> target" could not be simplified most of the time by a "source (dockerimage) -> target". I'm realizing this on most of my use cases: since the introduction of the "dockerimage" source, the need to check for image existence is clearly less common. Do you have the same analysis or do you see other angle or use cases?

• @olblak A part of my brain just reminded me about Handle different exit code #233. I believe that could help to provide a user-level choice using CLI flag (and not manifest changes). Some examples:

  • updatecli diff ./manifest.yaml => exit code would be 0 is one of the conditions is false and the targets are skipped
  • updatecli diff --detailed-exit-code ./manifest.yaml => exit code would be 2 is one of the conditions is false and the targets are skipped
  • updatecli diff --fail-on-skip ./scenario1.yaml => exit code would be 1 is the conditions are false and the targets skipped

[dduportal]

Thanks for taking care guys!

I do not have much context around updatecli code, but It might be necessary to think about what means failure/error in order to implement an error/exception control.

For Instance: Specifiying a non existing docker repository could mean failure/error

True that: @olblak don't hesitate, if you can remember or cross path with the uses cases you mentioned (e.g. skipped targets but need to investigate) to list them here.

I agree with @treezio : each plugin implementing a condition should properly separate:

• Error return (e.g. err != nil): missing parameter, authentication error, network or I/O problem, HTTP/5xx error (e.g. server errors), HTTP/400, 401, 403 errors
• False return (e.g. err == nil and `return != 0): the condition ran successfully but did not validated the hypothesis it is meant to check (HTTP/404, docker image does not exist, helm chart does not exist, etc.)
• True return (e.g. err == nil and `return == 0): the condition ran successfully and validated the hypothesis it is meant to check (HTTP/2xx, docker image found, etc.)

Does it make sense?

[olblak]

Closing as the current issue solve