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

add commit hash implementation similar to kuadrantctl to authorino #473

Conversation

ehearneRedHat
Copy link
Contributor

@ehearneRedHat ehearneRedHat commented Jul 1, 2024

What:

  • Makefile now contains implementation of git commit hash dirty implementation.
  • Dockerfile has fallback option for when git is not installed such as in a container running the image.
  • build arg changed in build images action to reflect implementation.
  • New build.yaml file to add build information for authorino compile. Currently just supports version .
  • RELEASE docs have been updated with instructions on how to push a release .

Verify:

NOTE: for running the binaries use authorino version to check implementation. Within container in default namespace, check the logs at the top for the version.

  1. Run make build and make local-setup locally. Without any file changes made the hash should be displayed without appendixed dirty flag. Then add a comment to a file to have git detect new changes and re-run the commands checking binary and in-cluster for both respectively.
  2. Run the build-images.yaml workflow and check that the binaries show the hash.

@ehearneRedHat ehearneRedHat requested a review from eguzki July 1, 2024 09:32
@ehearneRedHat ehearneRedHat self-assigned this Jul 1, 2024
.gitignore Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@ehearneRedHat ehearneRedHat requested a review from eguzki July 4, 2024 08:58
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@ehearneRedHat ehearneRedHat force-pushed the 471-add-commit-hash-implementation-similar-to-kuadrantctl-to-authorino branch from 47b54f9 to 9d217f5 Compare July 15, 2024 12:15
Makefile Outdated Show resolved Hide resolved
@ehearneRedHat ehearneRedHat force-pushed the 471-add-commit-hash-implementation-similar-to-kuadrantctl-to-authorino branch from 110503e to 8b7a07d Compare July 26, 2024 12:18
@ehearneRedHat
Copy link
Contributor Author

@guicassolato re-requesting your review... I didn't add the version to the workflow file as requested .

@guicassolato
Copy link
Collaborator

@ehearneRedHat, I think I'm missing something...

I still see the Authorino version with -dev suffix hard-coded in the Makefile for the docker-build arg. I thought we'd be reading it from the build.yaml. 🤔

The build-images workflow on the other hand seems to be setting VERSION always to latest, even when we build for feature branches and tag releases!

I guess what I expected was:

main feature branch vX.Y.Z
Version latest <branch-name> X.Y.Z
Image tags latest, <git-sha> <branch-name>, <git-sha> vX.Y.Z, <git-sha>

It should not matter if we build the image locally (dev env) or in a GHA. The outcome should be the same, depending only on what we are building, i.e., the code currently in main, in a feature branch, or "vX.Y.Z"-tagged (released version).

The build.yaml file should be just a resource to achieve that, if needed and to avoid hard-coding version numbers inside a Makefile or a Golang file.

Another scenario – which I do not like, but could live with if the rest of Kuadrant so decides:

main feature branch vX.Y.Z
Version <curr+1>-dev <curr+1>-dev X.Y.Z
Image tags latest, <git-sha> <branch-name>, <git-sha> vX.Y.Z, <git-sha>

@ehearneRedHat
Copy link
Contributor Author

@ehearneRedHat, I think I'm missing something...

I still see the Authorino version with -dev suffix hard-coded in the Makefile for the docker-build arg. I thought we'd be reading it from the build.yaml. 🤔

The build-images workflow on the other hand seems to be setting VERSION always to latest, even when we build for feature branches and tag releases!

I guess what I expected was:

main feature branch vX.Y.Z
Version latest X.Y.Z
Image tags latest, , vX.Y.Z,
It should not matter if we build the image locally (dev env) or in a GHA. The outcome should be the same, depending only on what we are building, i.e., the code currently in main, in a feature branch, or "vX.Y.Z"-tagged (released version).

The build.yaml file should be just a resource to achieve that, if needed and to avoid hard-coding version numbers inside a Makefile or a Golang file.

Another scenario – which I do not like, but could live with if the rest of Kuadrant so decides:

main feature branch vX.Y.Z
Version <curr+1>-dev <curr+1>-dev X.Y.Z
Image tags latest, , vX.Y.Z,

@guicassolato you're absolutely right! I did forget to amend the docker-build target.

My understanding was that you would prefer not touching the version within the build-images.yaml workflow . That's why I didn't adjust.

I can see now what you were expecting, and how the current state isn't aligned with that! I think it sounds like a good idea. There would need to be some logic to check for the branches but I'll implement it!

@ehearneRedHat
Copy link
Contributor Author

@guicassolato I believe I made the requested changes. Feel free to review when you have the chance :)

@ehearneRedHat ehearneRedHat requested a review from eguzki August 19, 2024 12:33
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@ehearneRedHat
Copy link
Contributor Author

@guicassolato feel free to re-review again, but I need to check the verification steps first. You are too fast ! :D

@ehearneRedHat
Copy link
Contributor Author

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
hack/check-version.sh Outdated Show resolved Hide resolved
main.go Show resolved Hide resolved
hack/check-version.sh Outdated Show resolved Hide resolved
@ehearneRedHat
Copy link
Contributor Author

@guicassolato I have amended changes - feel free to review. :)

Comment on lines +37 to +42
tag=${GITHUB_REF_NAME/\//-}
echo "version=${tag#v}" >> $GITHUB_ENV
elif [[ ${GITHUB_REF_NAME/\//-} == "main" ]]; then
echo "version=latest" >> $GITHUB_ENV
else
echo "VERSION=${{ github.sha }}" >> $GITHUB_ENV
echo "version=${{ github.ref_name }}" >> $GITHUB_ENV
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we still have 2 ways to do the same thing more or less. Even though GHA and Makefile both seem to use the git ref, for some reason one does it straightforward and the other uses a file (two files!) in the process.

This GHA workflow (used to build and push the images to registry on pushes to main, releases and for PR testing) uses only the git ref straight away to infer the version info. The Makefile (used locally for dev/testing workflows), on the other hand, reads the version info from build-recent.yaml, while still generating this file (from a build.yaml "template") before every command, based on the git ref.

I see two paths from here to fix this. We need to decide whether build.yaml will be the source for the version info or not.

OPTION A: build.yaml is the source of version info:

  1. The version info should NOT be updated before every command, but simply read from the build.yaml file locally (this is the base line, but keep reading to see the special case)
  2. Updating build.yaml must be a manual step only performed in preparation for releases
  3. We need to start doing release branches for Authorino, so these can contain the version of build.yaml with the altered version info inside (i.e. "X.Y.Z"), unlike all other branches (including main) whose build.yaml will always say "latest"
  4. The GHA must read the version info also from build.yaml
  5. There must be a way for the user to manually override the info in build.yaml without modifying the file. This will be the case mostly of when building from feature branches/PRs, both using the Makefile or via GHA. We probably don't need build-recent.yaml for that.

OPTION B: build.yaml is NOT the source:

  1. version info must be inferred from the git ref directly, both in the GHA and in the make targets:
git ref version info
main latest
feature branch-name
vX.Y.Z X.Y.Z

IMO, OPTION B looks much simpler. On the flip side, it doesn't give us the traceability, along with the code, about which version info would be stamped when building from that source. However, we're also introducing git shas and the dirty flag here. So I tend to think that storing the version info along with the code is not needed.

But please let me know what you think @ehearneRedHat @eguzki @didierofrivia

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guicassolato I think OPTION B looks to be easier but you would lose the traceability as you mentioned, but there would be less files to look to fix.

I will await other responses before implementing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The version info in logs gives high level view of the version being used. Useful to know if some feature should be there or not.

For us, developers, version is not accurate enough to debug and investigate issues. We want the git sha revision to know exactly the source code revision being run.

I leave version management up to you guys.

My take? version lives in version.go. In main branch it is vX.Y.Z-dev (where vX.Y.Z has not been released yet). Note that it is not latest or main as it is meaningless. In release branches it is vX.Y.Z. In RELEASE.md file, there is an additional step to update this version.go file. This is the approach we are taking in other kuadrant projects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@didierofrivia what are your thoughts ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Going with OPTION B here for simplicity.

@ehearneRedHat
Copy link
Contributor Author

I pass the baton over to @guicassolato to complete this task, as today marks my last day of the internship in Kuadrant at Red Hat.

Thank you to everyone who has helped me over the eight months of my being here. It was an amazing experience and hope to be back someday soon. ✌️

@guicassolato
Copy link
Collaborator

I pass the baton over to @guicassolato to complete this task, as today marks my last day of the internship in Kuadrant at Red Hat.

Thank you to everyone who has helped me over the eight months of my being here. It was an amazing experience and hope to be back someday soon. ✌️

Thank you so much for this, @ehearneRedHat! I will own the PR now, though the credit is still all yours, if you ask me!

@guicassolato guicassolato requested a review from a team September 3, 2024 12:35
alexsnaps
alexsnaps previously approved these changes Sep 10, 2024
Copy link
Member

@alexsnaps alexsnaps left a comment

Choose a reason for hiding this comment

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

Minor questions... but this should do the job

Comment on lines 45 to 54
- name: Set Authorino Dirty
id: authorino-dirty
run: |
GIT_DIRTY=$(git diff --stat); \
if [ -n "$GIT_DIRTY" ]; then \
dirty="true"; \
else \
dirty="false"; \
fi; \
echo "dirty=$dirty" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

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

Confused 🤔 How can this ever be dirty? Isn't this always a straight pull from a git ref and built from there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point. I don't think it can ever be dirty.

Dockerfile Show resolved Hide resolved
Signed-off-by: ehearneredhat <[email protected]>
Signed-off-by: Guilherme Cassolato <[email protected]>
@guicassolato guicassolato force-pushed the 471-add-commit-hash-implementation-similar-to-kuadrantctl-to-authorino branch from b536533 to 7a7d904 Compare September 12, 2024 13:46
@guicassolato guicassolato merged commit 7d07f22 into main Sep 12, 2024
14 checks passed
@guicassolato guicassolato deleted the 471-add-commit-hash-implementation-similar-to-kuadrantctl-to-authorino branch September 12, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add commit hash dirty implementation, similar to kuadrantctl, to authorino.
5 participants