Makefile, push: Prevent overwriting existing version tags

[RamLavi]

What this PR does / why we need it:
The IMAGE_GIT_TAG is generated using git describe to create a virtual
tag for the image, and used in order to tag every push to the repository
for later use.
However, when an actual git tag exists (e.g., v0.45.0), git describe
returns that tag. This behavior makes it possible to accidentally
overwrite push an existing version tag in the registry.

Flow Leading to the Issue:

1. A new kmp release is created, pushing a new tag (e.g., v0.45.0).
2. A stable branch is created from that commit, pushing a new stable
   branch tag (e.g., release-0.45_latest).
   2.1 . During this push, IMAGE_GIT_TAG resolves to this Git tag (e.g.,
   v0.45.0) due to git describe.
   2.2 Makefile attempts to push the image with this tag (e.g., v0.45.0) to
   the registry, overwriting the original tag sha256 digest.

To address this, introducing a check to ensure such tags are not
overwritten, preserving the integrity of published versions.

Special notes for your reviewer:

Release note:

NONE

[oshoval]

Can it be simplified please to one target ? (something like this, but didn't check it is correct, just as a direction)

docker-push:
	@echo "Checking if tag '${IMAGE_GIT_TAG}' exists in ${REGISTRY}/${IMG}..."
	@if ! skopeo inspect docker://${REGISTRY}/${IMG}:${IMAGE_GIT_TAG} >/dev/null 2>&1; then \
	    echo "Tag '${IMAGE_GIT_TAG}' does not exist. Pushing image..."; \
	    $(OCI_BIN) push ${TLS_SETTING} ${REGISTRY}/${IMG}:${IMAGE_TAG}; \
	    $(OCI_BIN) tag ${REGISTRY}/${IMG}:${IMAGE_TAG} ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}; \
	    $(OCI_BIN) push ${TLS_SETTING} ${REGISTRY}/${IMG}:${IMAGE_GIT_TAG}; \
	else \
	    echo "Tag '${IMAGE_GIT_TAG}' already exists. Skipping push."; \
	fi

[oshoval]

btw it is raceful
lets say skopeo due to hiccup (but lets say tag exists), and then it entered the good path, and can override
need to assert the error of skopeo is not found, not any other error please

[RamLavi]

sure, DONE

[RamLavi]

DONE.
My only worry is that if they change the error format then it might break, but I'm over analyzing this thing, so let's just keep it like this

[oshoval]

sgtm
only thing that worth considering later if desired is if there are any side effects in case we match the last assertion
(when all the other branches didnt meet)

[oshoval]

Please add in the description the flow of what you done that created it, it will help understanding when it happens and the solution / it's correctness

[RamLavi]

Please add in the description the flow of what you done that created it, it will help understanding when it happens and the solution / it's correctness

DONE, PTAL

[oshoval]

Thanks please add the info you added to the commit to the PR desc

it means in this case we will only have release-0.45_latest for the branch right ?
i would say it is a bit confusing, and hopefully all the automation in case that it will need to read it will support it and wont be confused, best to always create patch after that on the branch maybe

Worth to consider please to think how we can create releases on main that wont affect future branching
but this is not for this scope, and no rush i guess

There is also the possible race above, please take a look
might worth to even inspect as the first operation, unless the side effects that are done are neglectable,
else the script should first inspect, see that it can have the answer and then do the rest

[RamLavi]

Thanks please add the info you added to the commit to the PR desc

it means in this case we will only have release-0.45_latest for the branch right ? i would say it is a bit confusing, and hopefully all the automation in case that it will need to read it will support it and wont be confused, best to always create patch after that on the branch maybe

Worth to consider please to think how we can create releases on main that wont affect future branching but this is not for this scope, and no rush i guess

yes, for follow up for sure.

There is also the possible race above, please take a look might worth to even inspect as the first operation, unless the side effects that are done are neglectable, else the script should first inspect, see that it can have the answer and then do the rest

fixed. PTAL

[oshoval]

Do we need exit 1 so we will know it happens ?
needed if the tag can be mandatory on some cases (which i believe it does)

[RamLavi]

It's not mandatory but it doesn't hurt to know it failed on the gitActions.. Otherwise we'll never see it.
DONE

[oshoval]

Thanks
/lgtm

[oshoval]

/bin/sh: -c: line 9: syntax error: unexpected end of file
make[1]: *** [Makefile:96: docker-push] Error 2
make[1]: Leaving directory '/tmp/kubemacpool/kubemacpool'
make: *** [Makefile:113: cluster-sync] Error 2

[RamLavi]

/bin/sh: -c: line 9: syntax error: unexpected end of file
make[1]: *** [Makefile:96: docker-push] Error 2
make[1]: Leaving directory '/tmp/kubemacpool/kubemacpool'
make: *** [Makefile:113: cluster-sync] Error 2

my bad, forgot the \ after the exit 1

[oshoval]

Thanks
/lgtm

btw ; might not be needed when it is on multi line, but it is nit and also i didnt make sure about it

[RamLavi]

/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RamLavi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [RamLavi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[oshoval]

Error checking for tag 'v0.45.0-2-gbd72b46c'. Aborting to avoid potential overwrite.
make[1]: *** [Makefile:96: docker-push] Error 1
make[1]: Leaving directory '/tmp/kubemacpool/kubemacpool'
make: *** [Makefile:113: cluster-sync] Error 2
+ teardown

[RamLavi]

For some reason it doesn't catch it, while locally it works:

$ IMAGE_GIT_TAG=v0.45.0-2-g089d9c26 make docker-push
Tagging 'v0.45.0-2-g089d9c26'!
$ IMAGE_GIT_TAG=latest make docker-push
Tag 'latest' already exists. Skipping tagging and push.

[oshoval]

maybe old skopeo ?
worth to ask Brian maybe please
or to try to dirty install new skopeo first to see if it helps

[RamLavi]

maybe old skopeo ?

could be. I'm thinking we drop the check, and if we see a race in the future we tackle it.
The IMAGE_GIT_TAG is not important for prod

[oshoval]

if you drop the whole check, doesnt it mean it will always give wrong answer?
we do need a basic condition there right ?
and that one need to be correct
(worth to check what it prints etc on CI)

[RamLavi]

@oshoval fixed the issue, PTAL