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

release: Update KServe/dependencies #379

Merged
merged 17 commits into from
May 31, 2023
Merged

Conversation

rafvasq
Copy link
Member

@rafvasq rafvasq commented May 24, 2023

Motivation

Update KServe and dependencies in preparation for v0.11.0 release

Modifications

  • Update go.mod dependencies
  • Update CRDs under config/crd/bases
  • Remove outdated module replacements
  • Update Dockerfile.develop:
    • Use Go 1.19
    • Fix broken controller-gen install
    • Add safe.directory work-around for the git dubious ownership error during the GHA lint workflow
  • Temporarily disable lint deprecation check "SA1019"
  • Update Copyright header formatting in *.go sources to not be mistaken as package documentation
  • Update mock client GETtion func signature in grpc_resolver_test.go

Result

Testing KServe RC in preparation for release

@rafvasq rafvasq added this to the v0.11.0 milestone May 24, 2023
@rafvasq rafvasq requested review from ckadner and njhill May 24, 2023 18:25
Signed-off-by: Rafael Vasquez <[email protected]>
@ckadner ckadner marked this pull request as draft May 24, 2023 18:44
@ckadner ckadner self-assigned this May 24, 2023
@ckadner
Copy link
Member

ckadner commented May 24, 2023

@rafvasq -- I marked this as a draft until the go.mod changes are sorted out. I don't think we want/need to upgrade major package versions

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 @rafvasq -- I know your are still working on this, so just a few points for now

go.mod Show resolved Hide resolved
rafvasq added 2 commits May 24, 2023 14:58
Signed-off-by: Rafael Vasquez <[email protected]>
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 @rafvasq unless the KServe changes absolutely require it, I suggest we stick with go 1.18 for this release. Otherwise, there are too many subsequent changes we would have to make.

Dockerfile.develop Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
scripts/deploy/iks/test-fvt.sh Outdated Show resolved Hide resolved
Signed-off-by: Rafael Vasquez <[email protected]>
@ckadner
Copy link
Member

ckadner commented May 27, 2023

The build still failed locally even if I bring some of these dependencies back a version. The only way I was able to pass build/lint locally so far as been to upgrade to go 1.19 and golangci-lint to 1.52.2.

Right, I see now. KServe updated to Go 1.20 last week. So we are on the hook to follow suit. Sadly it's extremely late in the release cycle for such a change.

I can try to help with more changes required to accommodate that upgrade.

@rafvasq
Copy link
Member Author

rafvasq commented May 27, 2023

The build still failed locally even if I bring some of these dependencies back a version. The only way I was able to pass build/lint locally so far as been to upgrade to go 1.19 and golangci-lint to 1.52.2.

Right, I see now. KServe updated to Go 1.20 last week. So we are on the hook to follow suit. Sadly it's extremely late in the release cycle for such a change.

I can try to help with more changes required to accommodate that upgrade.

As discussed, we'll go for 1.19 after all because the go-toolset image we use is at 1.19

@ckadner
Copy link
Member

ckadner commented May 27, 2023

Sounds good. I will try to make some more changes and push to your fork

ckadner added 2 commits May 30, 2023 11:48
- Remove outdated module replacements
- Update Dockerfile.develop
- Fix broken controller-gen install
- Temporarily disable lint deprecation check "SA1019"
- Update Copyright header formatting to not be mistaken
  as package documentation
- Update mock client GET func signature in grpc_resolver_test.go

Signed-off-by: Christian Kadner <[email protected]>
@ckadner ckadner self-requested a review May 30, 2023 22:36
@ckadner ckadner dismissed their stale review May 30, 2023 22:37

Outdated

.github/workflows/lint.yml Outdated Show resolved Hide resolved
@ckadner ckadner marked this pull request as ready for review May 31, 2023 02:03
ckadner
ckadner previously approved these changes May 31, 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.

@njhill -- I made a few more changes to pass all the pull request checks. Please add your /lgtm /approve when you get a change

@ckadner ckadner assigned njhill and unassigned ckadner May 31, 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.

Adding a bit more context for some of the changes

.golangci.yaml Show resolved Hide resolved
pkg/mmesh/grpc_resolver_test.go Show resolved Hide resolved
Dockerfile.develop Show resolved Hide resolved
Dockerfile.develop Show resolved Hide resolved
@kserve kserve deleted a comment from kserve-oss-bot May 31, 2023
@kserve kserve deleted a comment from kserve-oss-bot May 31, 2023
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @ckadner.

Out of interest do you know if there were many additional issues going to go 20? Not suggesting we should do that for the release but would be a nice follow-on to align with core kserve.

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhill, rafvasq

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

@njhill
Copy link
Member

njhill commented May 31, 2023

/lgtm

@ckadner
Copy link
Member

ckadner commented May 31, 2023

Out of interest do you know if there were many additional issues going to go 20? Not suggesting we should do that for the release but would be a nice follow-on to align with core kserve.

We went with Go 1.19 (not Go 1.20) just because the go-toolset image we use is at 1.19

Once there is a UBI image for 1.20 we can do another upgrade.

@ckadner ckadner merged commit 4808804 into kserve:main May 31, 2023
lgdeloss pushed a commit to lgdeloss/modelmesh-serving that referenced this pull request Jun 5, 2023
Update KServe and dependencies in preparation for v0.11.0 release:
- Update go.mod
- Update CRDs under config/crd/bases
- Remove outdated module replacements
- Update Dockerfile.develop:
  - Use Go 1.19
  - Fix broken controller-gen install
- Add safe.directory work-around for the git 'dubious ownership'
  error during the GHA `lint` workflow
- Temporarily disable lint deprecation check "SA1019"
- Update Copyright header formatting in *.go sources to not be
  mistaken as package documentation
- Update mock client GET function signature in grpc_resolver_test.go

Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Co-authored-by: Christian Kadner <[email protected]>
Signed-off-by: Luis Delossantos <[email protected]>
lgdeloss pushed a commit to lgdeloss/modelmesh-serving that referenced this pull request Jun 6, 2023
Update KServe and dependencies in preparation for v0.11.0 release:
- Update go.mod
- Update CRDs under config/crd/bases
- Remove outdated module replacements
- Update Dockerfile.develop:
  - Use Go 1.19
  - Fix broken controller-gen install
- Add safe.directory work-around for the git 'dubious ownership'
  error during the GHA `lint` workflow
- Temporarily disable lint deprecation check "SA1019"
- Update Copyright header formatting in *.go sources to not be
  mistaken as package documentation
- Update mock client GET function signature in grpc_resolver_test.go

Signed-off-by: Rafael Vasquez <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Co-authored-by: Christian Kadner <[email protected]>
Signed-off-by: Luis Delossantos <[email protected]>
ckadner pushed a commit that referenced this pull request Sep 13, 2023
Address deprecation warnings after updating to Go 1.19 (#379)
by updating deprecated code to use generic Set instead.

Resolves #386

---------

Signed-off-by: Rafael Vasquez <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants