-
Notifications
You must be signed in to change notification settings - Fork 204
Update/go #265
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
base: master
Are you sure you want to change the base?
Update/go #265
Conversation
Co-authored-by: Yash Gangwar <[email protected]>
* updated release workflow & go version to fix vulns * updated release workflow perms --------- Co-authored-by: Yash Gangwar <[email protected]>
* updated release workflow & go version to fix vulns * updated release workflow perms * updated release workflow perms --------- Co-authored-by: Yash Gangwar <[email protected]>
Co-authored-by: Yash Gangwar <[email protected]>
* updated some modules versions to fix vulns * updated some modules versions to fix vulns --------- Co-authored-by: Yash Gangwar <[email protected]>
Co-authored-by: Yash Gangwar <[email protected]>
* updated some modules versions to fix vulns * updated some modules versions to fix vulns --------- Co-authored-by: Yash Gangwar <[email protected]>
Signed-off-by: yash-gangwar-aurva <[email protected]>
Signed-off-by: yash-gangwar-aurva <[email protected]>
* updated go to 1.24.4 * updated some modules versions to fix vulns * updated some modules versions to fix vulns --------- Signed-off-by: yash-gangwar-aurva <[email protected]>
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.
Whats the reason behind this PR? 1.23 is actively patched.
Dockerfile
Outdated
@@ -1,4 +1,4 @@ | |||
FROM golang:1.24 AS build | |||
FROM golang:1.24.5 AS build |
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.
Its not a good idea to pin here
.github/workflows/release.yml
Outdated
@@ -14,7 +18,7 @@ jobs: | |||
- name: Setup Go | |||
uses: actions/setup-go@v5 | |||
with: | |||
go-version: '1.23' | |||
go-version: '1.24.5' |
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.
Not a good idea to pin
permissions: | ||
contents: write | ||
packages: write |
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.
Whats this?
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.
These permissions were added to the workflow level in
so there is no need to add them to the job level also.
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 source branch https://github.com/aurva-io/grpc-health-probe/tree/update/go seems very outdated:
22 commits ahead of, 20 commits behind grpc-ecosystem/grpc-health-probe:master.
...and is in conflict with master
branch.
@yash-gangwar-aurva, I would suggest a rebase and resolving the conflicts before this PR can be looked into any further.
@@ -2,6 +2,7 @@ name: ci | |||
on: | |||
push: | |||
pull_request: | |||
workflow_dispatch: |
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.
Why do we add this?
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.
This would allow you to trigger the workflow manually on https://github.com/grpc-ecosystem/grpc-health-probe/actions/workflows/ci.yml (top right corner above the list).
Not sure why it would be needed though.
go.mod
Outdated
google.golang.org/genproto/googleapis/rpc v0.0.0-20241202173237-19429a94021a // indirect | ||
google.golang.org/protobuf v1.36.1 // indirect | ||
) | ||
|
||
go 1.23 | ||
go 1.24.2 |
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.
Could we update it to 1.24.5
Signed-off-by: yash-gangwar-aurva <[email protected]>
@@ -14,7 +18,7 @@ jobs: | |||
- name: Setup Go | |||
uses: actions/setup-go@v5 | |||
with: | |||
go-version: '1.23' | |||
go-version: '1.24.6' |
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.
Manually pinning to a version would be a regression for us.
@@ -1,4 +1,4 @@ | |||
FROM golang:1.24 AS build | |||
FROM golang:1.24.6 AS build |
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.
Manually pinning to a version would be a regression for us.
I don't think we should be pinning to specific Go versions. -1 on this as it stands. |
I see pinning exact Go versions as a way to have a change worthy of a release and newly built binaries with the pinned Go version. Similar could be achieved by creating releases without any code changes when necesarry (eg if a go release fixes a relevant bug). |
No description provided.