-
Notifications
You must be signed in to change notification settings - Fork 1
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 CMP release to version outputs #77
base: dev
Are you sure you want to change the base?
Conversation
Note Reviews pausedUse the following commands to manage reviews:
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve updates to versioning and logging in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
scripts/resolve_protocol_release.sh (1)
25-30
: Consider making the hash extraction more robust.The grep pattern for extracting the short hash might be too rigid.
Consider using a more flexible pattern:
-short_hash=$(grep -E "$dep_path" "$go_mod_path" | grep -oP '(\w{12})(?=\.)' || echo "") +short_hash=$(grep -E "$dep_path" "$go_mod_path" | grep -oE '[a-f0-9]{12,}' | head -n1 || echo "")internal/version/version.go (1)
36-47
: Consider improving the version string format.While the version string is comprehensive, it could be more readable.
Consider using a more structured format:
- FullVersion = fmt.Sprintf("%s (git: %s)\n\nlibs:\n %s: %s (%s)\n %s: %s (%s)\n %s: %s", + FullVersion = fmt.Sprintf(`Version: %s +Git Commit: %s + +Dependencies: + Protocol Buffers: + Commit: %s + Release: %s + gRPC: + Commit: %s + Release: %s + Contracts: + Commit: %s`, AppVersion, AppGitCommit, - "buf.build protocolbuffers ", BufBuildPBCommit, BufBuildPBCMPRelease, - "buf.build grpc ", BufBuildGRPCCommit, BufBuildGRPCCMPRelease, - "camino-messenger-contracts", ContractsGitCommit, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cmd/camino_messenger_bot.go
(2 hunks)internal/version/version.go
(2 hunks)scripts/build.sh
(1 hunks)scripts/constants.sh
(1 hunks)scripts/resolve_protocol_release.sh
(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/build.sh
[warning] 32-32: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 37-37: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 44-44: git_tag is referenced but not assigned.
(SC2154)
[warning] 45-45: git_commit is referenced but not assigned.
(SC2154)
[warning] 46-46: protocolbuffers_release is referenced but not assigned.
(SC2154)
[warning] 47-47: grpc_release is referenced but not assigned.
(SC2154)
scripts/constants.sh
[warning] 11-11: git_tag appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 14-14: grpc_release appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 15-15: protocolbuffers_release appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (9)
scripts/resolve_protocol_release.sh (1)
8-17
: LGTM: Input validation and path setup is well-implemented.
The script properly validates input arguments and sets up paths correctly.
scripts/constants.sh (1)
11-11
: LGTM: Good addition of --dirty flag.
The --dirty flag helps track uncommitted changes in the working directory.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 11-11: git_tag appears unused. Verify use (or export if used externally).
(SC2034)
internal/version/version.go (1)
13-14
: LGTM: Clear naming for protocol release variables.
The new variables follow the existing naming convention and clearly indicate their purpose.
scripts/build.sh (4)
16-23
: LGTM! Clean and clear argument parsing.
The command-line argument parsing is well-structured with appropriate error handling for unknown options.
51-52
: LGTM! Proper version information injection.
The new LDFLAGS correctly inject the protocol buffer and gRPC CMP release information into the binary.
57-63
: LGTM! Improved build command visibility.
The build command construction and execution is now more transparent, making it easier to debug build issues.
43-47
: Verify version variables are properly defined.
The script references several version-related variables that should be defined in constants.sh.
Let's verify the variable definitions:
✅ Verification successful
Let me check the actual content of constants.sh to verify the variable definitions.
All version variables are properly defined
The variables used in the echo statements are correctly defined in constants.sh:
git_commit
andgit_tag
are set using git commands with fallback valuesprotocolbuffers_release
andgrpc_release
are set using the resolve_protocol_release.sh scriptDEBUG
appears to be an environment variable, which is a common practice
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if version variables are properly defined in constants.sh
# Find and check constants.sh content
echo "Checking constants.sh for variable definitions..."
fd -t f "constants.sh" -x grep -l "git_tag\|git_commit\|protocolbuffers_release\|grpc_release"
Length of output: 226
Script:
#!/bin/bash
# Check the content of constants.sh for these specific variables
echo "Content of constants.sh:"
cat ./scripts/constants.sh | grep -A 1 -E "(DEBUG|git_tag|git_commit|protocolbuffers_release|grpc_release)"
Length of output: 572
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 44-44: git_tag is referenced but not assigned.
(SC2154)
[warning] 45-45: git_commit is referenced but not assigned.
(SC2154)
[warning] 46-46: protocolbuffers_release is referenced but not assigned.
(SC2154)
[warning] 47-47: grpc_release is referenced but not assigned.
(SC2154)
cmd/camino_messenger_bot.go (2)
19-19
: LGTM! Enhanced version information.
Using FullVersion
provides more comprehensive version information, which is beneficial for debugging and support.
62-63
: LGTM! Improved version logging.
The logging now includes CMP release information alongside commit hashes, providing better context about the protocol versions in use.
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Dockerfile (1)
Line range hint
1-1
: Fix incorrect Go version in base imageThe base image
golang:1.23.1-alpine3.20
specifies a non-existent Go version. The latest Go version is 1.21.x.Apply this diff to fix the version:
-FROM golang:1.23.1-alpine3.20 AS build-stage +FROM golang:1.21.5-alpine3.20 AS build-stagescripts/constants.sh (1)
4-4
: Export the SCRIPT_DIR variable for external use.Since this variable is used to resolve paths in other scripts, it should be exported.
-SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd) +export SCRIPT_DIR=$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" &>/dev/null && pwd)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Dockerfile
(1 hunks)scripts/constants.sh
(1 hunks)scripts/resolve_protocol_release.sh
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/resolve_protocol_release.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/constants.sh
[warning] 9-9: current_branch appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 12-12: git_commit appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 13-13: git_tag appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 16-16: grpc_release appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 17-17: protocolbuffers_release appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (4)
Dockerfile (2)
11-11
: LGTM: Package additions are appropriate
Adding git
is required for submodule operations, and the addition of these packages in a single layer helps minimize the image size.
13-13
: Verify if bash is required for the build script
While the change from sh
to bash
works, consider if bash-specific features are actually needed. Using sh
could potentially reduce the image size if bash features aren't necessary.
Let's check if the build script uses bash-specific features:
✅ Verification successful
Let me check the complete content of the build script to better understand its usage of bash features.
Let me check the content of the constants.sh file as well since it's sourced by build.sh.
Bash is required for the build script
The build script uses several bash-specific features that are not available in POSIX sh:
[[
conditional expressions for argument parsing${BASH_SOURCE[0]}
array variable for script path resolutionset -euo pipefail
in constants.sh- Process substitution with
&>/dev/null
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if build.sh uses bash-specific features
# Look for bash-specific syntax in build.sh
echo "Checking for bash-specific features in build.sh:"
rg -n '(\[\[|\]\]|declare\s|-o pipefail|read\s+-[^r]|<<<|process\s+substitution|\$\{[a-zA-Z_][a-zA-Z0-9_]*:.*\})' ./scripts/build.sh
Length of output: 269
Script:
#!/bin/bash
# Show the complete content of build.sh
cat ./scripts/build.sh
Length of output: 1961
Script:
#!/bin/bash
cat ./scripts/constants.sh
Length of output: 864
scripts/constants.sh (2)
16-17
: Export the protocol release variables.
These variables are used externally and need to be exported.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 16-16: grpc_release appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 17-17: protocolbuffers_release appears unused. Verify use (or export if used externally).
(SC2034)
16-17
: Verify the resolve_protocol_release.sh script.
Ensure that the referenced script exists and has proper permissions.
✅ Verification successful
Script exists and has correct permissions
The resolve_protocol_release.sh
script exists in the scripts
directory and has executable permissions (-rwxr-xr-x
). The script is properly referenced in constants.sh
and is ready to be used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the script exists and is executable
if [ -f "${SCRIPT_DIR}/resolve_protocol_release.sh" ]; then
ls -l "${SCRIPT_DIR}/resolve_protocol_release.sh"
else
echo "Error: resolve_protocol_release.sh not found"
exit 1
fi
Length of output: 149
Script:
#!/bin/bash
# First, let's find the script location using fd
fd resolve_protocol_release.sh
# Let's also check the scripts directory structure
ls -la scripts/
Length of output: 956
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 16-16: grpc_release appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 17-17: protocolbuffers_release appears unused. Verify use (or export if used externally).
(SC2034)
internal/version/version.go
Outdated
AppVersion = "Unspecified" | ||
|
||
// AppGitCommit is set by go build -ldflags | ||
// set by go build -ldflags |
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.
I did that comment per variable, because that in interpreted as variable description by IDEs
maybe you can try to make var() block and comment whole block
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.
Ah ok, sounds logical. I've added the comments to the vars.
@coderabbitai ignore |
✅ Actions performedReviews paused. |
This PR adds protocol release label from buf.build to the compiled binary. It also makes the build process more explicit by providing information about the build to the stdout.
I've also updated how the
git_tag
is build withgit describe
, explanation below.Example
-v
output:Here we see the version is
v10.1.0-22-g8a20f6e-dirty (git: 8a20f6e)
, which means:v10.1.0
-22
part (this is only added if the current HEAD does not have a tag)8a20f6e
which comes from git (the-g
part before the SHA) (only if there is no tag for current HEAD)-dirty
part)(git: 8a20f6e)
this part is always added, even if current HEAD has a specific tag, in which case the version would be only the tag (if no dirty), ie:v10.1.0
This process ensures that we can identify for pretty much all the cases that can affect the binary. Previously it was always setting the version to tag name even if you had commits after that and the dirty workdir was not used at all.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores
bash
for script execution, improving compatibility with script constructs.