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

Single source of truth for go version #138

Merged
merged 27 commits into from
Nov 22, 2023
Merged

Conversation

geoff-vball
Copy link
Contributor

@geoff-vball geoff-vball commented Nov 16, 2023

Why this should be merged

Makes the go.mod file the single source of truth for the go version.

Adds a github action to check the go.mod file for any changes after a go mod tidy

Also augments versions.sh to get the versions from go.mod so we have a single source of truth for those too!

How this works

go.mod only allows major.minor version, so I've added a comment with the patch version that we will extract with sed in order to install the correct golang version in docker and CI.

How this was tested

How is this documented

@@ -40,9 +42,6 @@ jobs:
cd subnet-evm
./scripts/build.sh /tmp/e2e-test/avalanchego/plugins/srEXiWaHuhNyGwPUi444Tu47ZEDwxTWrbQiuD7FmgSAQ6X7Dy

- name: Checkout teleporter repository
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already done above on line 23

scripts/versions.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@feuGeneA feuGeneA left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for doing this!

"Requested changes" for a single character in gomod_checker.yml haha. The other comments are questions for my own education as much as for improvement of this PR.

.github/workflows/gomod_checker.yml Outdated Show resolved Hide resolved
scripts/utils.sh Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Made one small request.

scripts/versions.sh Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link

@minghinmatthewlam minghinmatthewlam left a comment

Choose a reason for hiding this comment

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

Generally LGTM, looks like the test.yml is complaining about relayer version format

cam-schultz
cam-schultz previously approved these changes Nov 21, 2023
Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

I don't love the pattern of using a value set within a comment throughout the code base. At the least, lets add a clear explanation to the comment regarding how it must be formatted and how it's used, but I think I would be in favor of just setting the full version explicitly in versions.sh and having a comment there and in the go.mod that it should be updated in both places when needed.

That's similar to how AvalancheGo approaches it here: https://github.com/ava-labs/avalanchego/blob/master/go.mod#L3

feuGeneA
feuGeneA previously approved these changes Nov 22, 2023
Copy link
Contributor

@cam-schultz cam-schultz left a comment

Choose a reason for hiding this comment

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

One last small request, then LGTM

scripts/versions.sh Outdated Show resolved Hide resolved
scripts/versions.sh Outdated Show resolved Hide resolved
cam-schultz
cam-schultz previously approved these changes Nov 22, 2023
@geoff-vball geoff-vball merged commit ee1fd7d into main Nov 22, 2023
13 checks passed
@geoff-vball geoff-vball deleted the gstuart/diff-go-mod-sum branch November 22, 2023 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants