-
Notifications
You must be signed in to change notification settings - Fork 63
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
chore: Upgrade Golang version from 1.17 to 1.19 #68
Conversation
chore: Go lang 1.17 and a little bit old and 1.19 as well. However, before taking a greater step to higher version would might be better to break it in smaller changes since the amount of changes that this update has required is a little bit high, here is a small description of what was changed by this update: - Outdated linters: - WARN [runner] The linter 'deadcode' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. - WARN [runner] The linter 'structcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. - WARN [runner] The linter 'structcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. - Deprecation of the io/ioutils package: - SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os] - Used the `os` package and the needed code changes applied - At puller/config.go - ( undefined: GetEnvString) - Fixed by `import . "github.com/kserve/modelmesh-runtime-adapter/internal/envconfig"` - Some comments and headers adjusted - small identation changes made as part of go fmt. Fixes kserve#68 Signed-off-by: Spolti <[email protected]>
1b40deb
to
d2167da
Compare
@ckadner could you please review this PR? |
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.
Thanks @spolti for going through the tedious process of updating all the code! I just have a few questions/suggestions.
a2b7326
to
70ce114
Compare
Hi @ckadner, it is ready for re-review :) |
chore: Go lang 1.17 and a little bit old and 1.19 as well. However, before taking a greater step to higher version would might be better to break it in smaller changes since the amount of changes that this update has required is a little bit high, here is a small description of what was changed by this update: - Outdated linters: - WARN [runner] The linter 'deadcode' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. - WARN [runner] The linter 'structcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. - WARN [runner] The linter 'structcheck' is deprecated (since v1.49.0) due to: The owner seems to have abandoned the linter. Replaced by unused. - Deprecation of the io/ioutils package: - SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os] - Used the `os` package and the needed code changes applied - At puller/config.go - ( undefined: GetEnvString) - Fixed by `import . "github.com/kserve/modelmesh-runtime-adapter/internal/envconfig"` - Some comments and headers adjusted - small identation changes made as part of go fmt. Fixes kserve#64 Signed-off-by: Spolti <[email protected]>
Signed-off-by: Spolti <[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.
Thanks @spolti -- 2 follow-ups from my previous review
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: spolti The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Tested with quickstart, no issues! FYI @ckadner
Signed-off-by: Spolti <[email protected]>
@spolti, simply meant that I deployed MM with your updates to this component and deployed/inferenced an inference service as outlined in the quickstart guide :) |
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.
So close! :-)
Oh, right, quickstart :) |
Signed-off-by: Spolti <[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.
Thanks @spolti -- just need some minor cleanup.
d6e9d80
to
cf4e67c
Compare
Signed-off-by: Spolti <[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.
Hi @spolti -- okay, I understand that either go fmt
will replace the spaces with tabs even in files that don't necessarily needed changing, or we/you add the empty line above the package declaration in all *.go
files. It's not worth another round of review. Let's do a sweep of all files after this PR.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ckadner, spolti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thank you @spolti ! |
Hi @ckadner, I've created this issue to track the headers change: #75 |
Actually you've opened already, closed mine and add more information on #74 |
- Remove the linters for "deadcode", "structcheck", "varcheck" - Use "os" packages instead of deprecated "io/ioutil" (SA1019) - Capture pre-commit output in a local log file Fixes opendatahub-io#64 --------- Signed-off-by: Spolti <[email protected]>
chore: Go lang 1.17 is a little bit old and 1.19 as well. However, before taking a greater step to higher version would might be better to break it in smaller changes since the amount of changes that this update has required is a little bit high, here is a small description of what was changed by this update:
Outdated linters:
Deprecation of the io/ioutils package:
os
package and the needed code changes appliedAt puller/config.go
import . "github.com/kserve/modelmesh-runtime-adapter/internal/envconfig"
Some comments and headers adjusted
small identation changes made as part of go fmt.
Fixes #64