-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Linter checks go versions in Dockerfile and YAML files #8954
Conversation
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 as PR comments)
Additionally, you can add 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.
Looks good. But it seems like the linter now crashes, due to the Golang version update...
ca74929
to
8f34168
Compare
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.
Very nice, LGTM 🎉
8f34168
to
8d2693a
Compare
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.
got this failure on my local machine,
make check-go-version
Checking for target Go version (v1.22.5) in Dockerfile files (*Dockerfile)
./scripts/check-go-version-dockerfile.sh 1.22.5
Skipping ./tools/Dockerfile
./docker/btcd/Dockerfile is using Go version 1.22.5.
./Dockerfile is using Go version 1.22.5.
./dev.Dockerfile is using Go version 1.22.5.
./make/builder.Dockerfile is using Go version 1.22.5.
./lnrpc/Dockerfile is using Go version 1.22.5.
All Dockerfiles pass the Go version check for Go version 1.22.5.
Checking for target Go version (v1.22.5) in YAML files (*.yaml, *.yml)
./scripts/check-go-version-yaml.sh 1.22.5
Error: ./.github/workflows/release.yaml specifies Go version '', but not version '1.22.5'.
make: *** [check-go-version-yaml] Error 1
Makefile
Outdated
@$(call print, "Linting source.") | ||
$(DOCKER_TOOLS) golangci-lint run -v $(LINT_WORKERS) | ||
|
||
#? lint: Run static code analysis | ||
lint: lint-source check-go-version |
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.
should these two be switched to give much quicker feedback? I also wonder if it's better to have an independent version check in the CI, but I guess not since the linter also depends on the go version?
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've switched these around.
I'm not sure if we need an independent check. Would be good to have a single Go version to update for all checks. Depends how independent you mean.
536cb87
to
f108730
Compare
@yyforyongyu thanks for the review! I think this might be a difference in |
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.
Stil has an error tho - will figure out how to parse it in mac
local yamlfile="$1" | ||
local required_go_version="$2" | ||
|
||
# Use grep to find lines with 'go:'. The grep exist status is ignored. |
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.
do we use the prefix go:
? I thought we always use GO_VERSION:
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 think the only example is in .golangci.yml
:
staticcheck:
go: "1.21"
checks: ["-SA1019"]
And I don't think even care about that case. See: #8954 (comment)
Do we keep this check anyway because it might come into play and be useful later?
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.
Ok, in the latest commits, I've bumped the Go version in .golangci.yml
. And I've modified the lint check so that it's included in the check.
# GO_VERSION: '1.21.0' | ||
# GO_VERSION: 1.21.0 | ||
# GO_VERSION:1.21.0 | ||
# GO_VERSION:1.21.0 |
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.
nit: remove the spaces
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 leading spaces here are intentional. We need to be robust to leading white space.
Renamed the Makefile variable `GO_VERSION` to `ACTIVE_GO_VERSION` for improved clarity. This change also frees up the name `GO_VERSION` to be used for defining the global Go version for reproducible binaries in a subsequent commit.
f108730
to
ced1f04
Compare
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 got this error,
Checking for target Go version (v1.22.5) in YAML files (*.yaml, *.yml)
./scripts/check-go-version-yaml.sh 1.22.5
grep: invalid option -- P
By applying this diff it now works on a mac,
diff --git a/scripts/check-go-version-yaml.sh b/scripts/check-go-version-yaml.sh
index a8469abdc..af4bdbfa8 100755
--- a/scripts/check-go-version-yaml.sh
+++ b/scripts/check-go-version-yaml.sh
@@ -13,11 +13,11 @@ check_go_version_yaml() {
if [ -n "$go_lines" ]; then
# Extract the Go version from the file's lines. Example matching strings:
# go: "1.21.0"
- local extracted_go_version=$(echo "$go_lines" | grep -oP '^\s*go:\s*"\K[^"]+')
+ local extracted_go_version=$(echo "$go_lines" | sed -n 's/.*go: "\([^"]*\)".*/\1/p')
# Check if the extracted Go version matches the required version.
if [ "$extracted_go_version" != "$required_go_version" ]; then
- echo "Error: $yamlfile specifies Go version '$extracted_go_version', but not version '$required_go_version'."
+ echo "Error finding pattern 'go:': $yamlfile specifies Go version '$extracted_go_version', but not version '$required_go_version'."
exit 1
else
echo "$yamlfile specifies Go version $required_go_version."
@@ -43,11 +43,11 @@ check_go_version_env_variable() {
# GO_VERSION: 1.21.0
# GO_VERSION:1.21.0
# GO_VERSION:1.21.0
- local extracted_go_version=$(echo "$go_lines" | sed -n 's/^[[:space:]]*GO_VERSION:[[:space:]]*\(['"'"'"]\?\)\([0-9]\+\.[0-9]\+\.[0-9]\+\)\(['"'"'"]\?\)/\2/p')
+ local extracted_go_version=$(echo "$go_lines" | sed -n 's/.*GO_VERSION[: ]*["'\'']*\([0-9.]*\).*/\1/p')
# Check if the extracted Go version matches the required version.
if [ "$extracted_go_version" != "$required_go_version" ]; then
- echo "Error: $yamlfile specifies Go version '$extracted_go_version', but not version '$required_go_version'."
+ echo "Error finding pattern 'GO_VERSION:': $yamlfile specifies Go version '$extracted_go_version', but not version '$required_go_version'."
exit 1
else
echo "$yamlfile specifies Go version $required_go_version."
Implemented linter scripts to ensure consistency of the Go version specified in Dockerfiles and YAML files. These scripts verify that the Go version used across these files is uniform, enhancing maintainability and reducing configuration errors. This commit also introduces a `GO_VERSION` Makefile variable to control the Go version used throughout the project.
ced1f04
to
2a6e540
Compare
Thanks for the mac fix @yyforyongyu ! |
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.
LGTM🙏
Change Description
This PR adds two checks to the linter. These checks ensure that the Go version used in Dockerfile files and YAML files are consistent and as expected.
This PR also ensures that Go version 1.22.5 is used throughout the project.
(Note that I couldn't re-open #8211 I think it's because I forced pushed to the branch before re-opening?)
Steps to Test
Steps for reviewers to follow to test the change.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]
in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.