-
Notifications
You must be signed in to change notification settings - Fork 350
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
fix: replace the hardcoded default version string (#1836) #1900
Conversation
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 great! A few minor things to fix here.
Also, we should add some tests for this code as well.
|
||
// versionString indiciates the version of the proxy currently in use. | ||
// | ||
//go:embed version.txt |
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.
You'll need to move version.txt to this directory.
Also, we'll need to update the location of this file in .github/release-please.yml
.
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.
version.txt has been moved under proxy/util.
It seems to me that v1 branch doesn't have .github/release-please.yml file. Instead .build/release_artifacts.sh refers to version.txt. I'll update it that one.
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.
Jack told me offline that the release-please.yml file in main branch needs to be updated. So created this PR #1904. Please let me know if that looks OK. Thanks!
6d1fdb5
to
7cf85fb
Compare
7cf85fb
to
6589ef3
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.
LGTM ✅ Let's wait and let @enocom take a look on Monday before merging though
proxy/util/cloudsqlutil.go
Outdated
//go:embed version.txt | ||
var versionString string | ||
|
||
// metadataString indiciates additional build or distribution metadata. |
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.
s/indiciates/indicates/
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 catch! Fixed.
proxy/util/cloudsqlutil.go
Outdated
return v | ||
} | ||
|
||
// userAgentFromVersionString returns an appropriate user agent string for identifying this proxy process. |
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.
Let's try to wrap at 80 characters here.
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 catch! Fixed.
Remove empty line fixed cloudsqlutil.go format fix test code Fixed the typo in the test message Fixed typo and wrapped the long line
6589ef3
to
ad605cb
Compare
Replace hardcoded default version string with the one specified in version.txt.