-
Notifications
You must be signed in to change notification settings - Fork 32
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
add commit hash implementation similar to kuadrantctl to authorino #473
Conversation
47b54f9
to
9d217f5
Compare
110503e
to
8b7a07d
Compare
@guicassolato re-requesting your review... I didn't add the version to the workflow file as requested . |
@ehearneRedHat, I think I'm missing something... I still see the Authorino version with The build-images workflow on the other hand seems to be setting I guess what I expected was:
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 The Another scenario – which I do not like, but could live with if the rest of Kuadrant so decides:
|
@guicassolato you're absolutely right! I did forget to amend the My understanding was that you would prefer not touching the version within the 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! |
@guicassolato I believe I made the requested changes. Feel free to review when you have the chance :) |
@guicassolato feel free to re-review again, but I need to check the verification steps first. You are too fast ! :D |
@guicassolato I have amended changes - feel free to review. :) |
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 |
There was a problem hiding this comment.
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:
- 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) - Updating
build.yaml
must be a manual step only performed in preparation for releases - 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) whosebuild.yaml
will always say "latest" - The GHA must read the version info also from
build.yaml
- 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 needbuild-recent.yaml
for that.
OPTION B: build.yaml
is NOT the source:
- 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
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! |
There was a problem hiding this 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
.github/workflows/build-images.yaml
Outdated
- 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Signed-off-by: ehearneredhat <ehearne@redhat.com> Signed-off-by: Guilherme Cassolato <guicassolato@gmail.com>
b536533
to
7a7d904
Compare
What:
build.yaml
file to add build information for authorino compile. Currently just supportsversion
.Verify:
NOTE: for running the binaries use
authorino version
to check implementation. Within container indefault
namespace, check the logs at the top for the version.make build
andmake local-setup
locally. Without any file changes made the hash should be displayed without appendixeddirty
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.build-images.yaml
workflow and check that the binaries show the hash.