Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

makefile improvement #241

Merged
merged 1 commit into from
Jun 25, 2024
Merged

makefile improvement #241

merged 1 commit into from
Jun 25, 2024

Conversation

scott-cotton
Copy link
Member

The introduction of the use of git diff-index --quiet HEAD in the make target tag-release was incorrectly causing pushes of tags with the wrong image tag. I think this was the root cause of https://github.com/signadot/signadot/issues/4587 (although the image tags present in kustomize sources at e2e-v2.X are fixed by https://github.com/signadot/signadot/issues/4585, this PR fixes the cause)

This PR makes tag-release idempotent, rather than reverting the call to git diff-index, as it seems was intended by the introduction of git diff-index .

One other gotcha I found was that git diff-index --exit-code can indicate there are differences when those differences (eg file modification times) don't necessarily cause git commit to commit changes, so git diff --exit-code is used in the replacement script here.

@daniel-de-vera
Copy link
Contributor

The introduction of the use of git diff-index --quiet HEAD in the make target tag-release was incorrectly causing pushes of tags with the wrong image tag

@scott-cotton, I'm not sure I understand this, can you give explain it more deeply? (or give an example?)

@scott-cotton
Copy link
Member Author

The introduction of the use of git diff-index --quiet HEAD in the make target tag-release was incorrectly causing pushes of tags with the wrong image tag

@scott-cotton, I'm not sure I understand this, can you give explain it more deeply? (or give an example?)

According to the docs, git diff-index --quiet implies git diff-index --exit-code, which means that when there is a detected difference, it exits with code 1. The command in the Makefile

git diff-index --quiet HEAD || git commit -m tag-release-$(RELEASE_TAG) k8s/base

Thus never commits the kustomize changes from the previous line and yet it pushes the tag as if it had.

@daniel-de-vera
Copy link
Contributor

According to the docs, git diff-index --quiet implies git diff-index --exit-code, which means that when there is a detected difference, it exits with code 1. The command in the Makefile

git diff-index --quiet HEAD || git commit -m tag-release-$(RELEASE_TAG) k8s/base

Thus never commits the kustomize changes from the previous line and yet it pushes the tag as if it had.

Mmm, check this example:

ddv@ddvpc:/tmp$ git clone [email protected]:signadot/hotrod.git
Cloning into 'hotrod'...
remote: Enumerating objects: 1970, done.
remote: Counting objects: 100% (1315/1315), done.
remote: Compressing objects: 100% (677/677), done.
remote: Total 1970 (delta 694), reused 1052 (delta 557), pack-reused 655
Receiving objects: 100% (1970/1970), 16.30 MiB | 4.39 MiB/s, done.
Resolving deltas: 100% (895/895), done.
ddv@ddvpc:/tmp$ 
ddv@ddvpc:/tmp$ 
ddv@ddvpc:/tmp$ cd hotrod/
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ export RELEASE_TAG=v1.0.0
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ (cd k8s/base && kustomize edit set image signadot/hotrod:$RELEASE_TAG)
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ git diff
diff --git a/k8s/base/kustomization.yaml b/k8s/base/kustomization.yaml
index f65673c..1c0c3d1 100644
--- a/k8s/base/kustomization.yaml
+++ b/k8s/base/kustomization.yaml
@@ -11,4 +11,4 @@ resources:
 
 images:
 - name: signadot/hotrod
-  newTag: latest
+  newTag: v1.0.0
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ git diff-index --quiet HEAD || git commit -m tag-release-$RELEASE_TAG k8s/base
[main 9b4363b] tag-release-v1.0.0
 1 file changed, 1 insertion(+), 1 deletion(-)
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ 
ddv@ddvpc:/tmp/hotrod$ git log
commit 9b4363b652d90542498f22a2e83b1ddc3fcda592 (HEAD -> main)
Author: Daniel De Vera <[email protected]>
Date:   Thu Jun 20 12:10:24 2024 -0300

    tag-release-v1.0.0

commit 16ea7c27c03ac4d4cdb9df72e7996cf446cceba1 (origin/main, origin/HEAD)
Merge: 6142a19 74613ff
Author: Daniel De Vera <[email protected]>
Date:   Wed Jun 19 16:06:38 2024 -0300

    Merge pull request #240 from signadot/update-cypress-test
    
    Update of cypress tests + makefile fix

@scott-cotton
Copy link
Member Author

Well, I must have been mistaken. I had some runs which led me to believe the above analysis (I did a similar thing to your test with different results) but now I am unable to repro.

Apologies for the associated noise.

However, leaving PR open for

  • improved idempotency
  • bug fix using git diff instead of git diff-index

@scott-cotton scott-cotton changed the title makefile fix fix makefile improvement Jun 21, 2024
Copy link
Contributor

@daniel-de-vera daniel-de-vera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daniel-de-vera
Copy link
Contributor

@scott-cotton, can we merge this PR?

@scott-cotton scott-cotton merged commit 56456b4 into main Jun 25, 2024
6 checks passed
@scott-cotton scott-cotton deleted the fix-makefile-fix branch June 25, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants