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

chore: Upgrade Golang version from 1.17 to 1.19 #68

Merged
merged 5 commits into from
Nov 30, 2023
Merged

Conversation

spolti
Copy link
Contributor

@spolti spolti commented Nov 1, 2023

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:

    • 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 #64

spolti added a commit to spolti/modelmesh-runtime-adapter that referenced this pull request Nov 1, 2023
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]>
@spolti spolti force-pushed the issue64 branch 3 times, most recently from 1b40deb to d2167da Compare November 1, 2023 19:48
@Jooho
Copy link

Jooho commented Nov 9, 2023

@ckadner could you please review this PR?

Copy link
Member

@ckadner ckadner left a 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.

Makefile Outdated Show resolved Hide resolved
internal/util/fileutil.go Outdated Show resolved Hide resolved
model-mesh-mlserver-adapter/server/server.go Show resolved Hide resolved
model-mesh-ovms-adapter/server/modelmanager.go Outdated Show resolved Hide resolved
scripts/fmt.sh Outdated Show resolved Hide resolved
@spolti spolti force-pushed the issue64 branch 3 times, most recently from a2b7326 to 70ce114 Compare November 15, 2023 17:00
@spolti
Copy link
Contributor Author

spolti commented Nov 15, 2023

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]>
@rafvasq rafvasq self-requested a review November 22, 2023 20:43
@ckadner ckadner requested review from ckadner, tjohnson31415 and njhill and removed request for joerunde November 22, 2023 22:40
@ckadner
Copy link
Member

ckadner commented Nov 23, 2023

Hi @ckadner, it is ready for re-review :)

thanks @spolti -- I added 2 more comments to the threads from my previous review

@ckadner ckadner changed the title bump go version from 1.17 to 1.19 chore: Upgrade Golang version from 1.17 to 1.19 Nov 23, 2023
@kserve kserve deleted a comment from kserve-oss-bot Nov 23, 2023
Copy link
Member

@ckadner ckadner left a 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

scripts/fmt.sh Outdated Show resolved Hide resolved
internal/envconfig/envconfig.go Outdated Show resolved Hide resolved
@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: spolti
To complete the pull request process, please assign rafvasq after the PR has been reviewed.
You can assign the PR to them by writing /assign @rafvasq in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ckadner ckadner added this to the v0.11.2 milestone Nov 23, 2023
Copy link
Member

@rafvasq rafvasq left a 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
Copy link
Contributor Author

spolti commented Nov 23, 2023

Tested with quickstart, no issues! FYI @ckadner

Hi @rafvasq, thanks for testing.
Do you mind sharing about this tool you used, quickstart, is this a public tool, or?

@rafvasq
Copy link
Member

rafvasq commented Nov 23, 2023

Tested with quickstart, no issues! FYI @ckadner

Hi @rafvasq, thanks for testing. Do you mind sharing about this tool you used, quickstart, is this a public tool, or?

@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 :)

Copy link
Member

@ckadner ckadner left a comment

Choose a reason for hiding this comment

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

So close! :-)

.pre-commit-config.yaml Outdated Show resolved Hide resolved
internal/modelschema/modelschema.go Show resolved Hide resolved
scripts/fmt.sh Outdated Show resolved Hide resolved
@spolti
Copy link
Contributor Author

spolti commented Nov 28, 2023

service as outlined in the quickstart guide :)

Oh, right, quickstart :)

Copy link
Member

@ckadner ckadner left a 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.

.gitignore Outdated Show resolved Hide resolved
internal/envconfig/envconfig.go Outdated Show resolved Hide resolved
internal/util/fileutil.go Outdated Show resolved Hide resolved
@spolti spolti force-pushed the issue64 branch 3 times, most recently from d6e9d80 to cf4e67c Compare November 29, 2023 14:20
Copy link
Member

@ckadner ckadner left a 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

internal/modelschema/modelschema.go Show resolved Hide resolved
Copy link

oss-prow-bot bot commented Nov 30, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ckadner ckadner merged commit 0ba2f47 into kserve:main Nov 30, 2023
7 checks passed
@ckadner
Copy link
Member

ckadner commented Nov 30, 2023

Thank you @spolti !

@spolti spolti deleted the issue64 branch November 30, 2023 17:36
@spolti
Copy link
Contributor Author

spolti commented Nov 30, 2023

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

Hi @ckadner, I've created this issue to track the headers change: #75

@spolti
Copy link
Contributor Author

spolti commented Nov 30, 2023

Actually you've opened already, closed mine and add more information on #74

spolti referenced this pull request in spolti/modelmesh-runtime-adapter Jan 12, 2024
- 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update go-toolset from 1.17 to 1.19
5 participants