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.

Sign up for GitHub

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

Jump to bottom

remove old github.com/golang/protobuf #517

Merged
merged 1 commit into from
Nov 27, 2024
Merged

remove old github.com/golang/protobuf #517

merged 1 commit into from
Nov 27, 2024

Conversation

huww98
Copy link
Contributor

@huww98 huww98 commented Nov 24, 2024

gnostic-models has not been updated for over 1 year. Just use its latest main branch, as in gnostic repo

This should be one step forward to remove old github.com/golang/protobuf from the k8s ecosystem. We are not in a hurry, but this cleanup needs to be done sooner or later.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 24, 2024
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 24, 2024
@huww98
Copy link
Contributor Author

huww98 commented Nov 24, 2024

/cc @Jefftree

@Jefftree
Copy link
Member

Looks good, I just need to double check the kubernetes/kubernetes version bump that it doesn't cause any dependency issues.

@Jefftree
Copy link
Member

kubernetes/kubernetes#128982

@aojea
Copy link
Member

aojea commented Nov 26, 2024

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt November 26, 2024 18:45
@liggitt
Copy link
Member

liggitt commented Nov 26, 2024

can we request a tag on the gnostic-models repo so we don't have to switch from a tag to an untagged commit?

@huww98
Copy link
Contributor Author

huww98 commented Nov 26, 2024

There is a request google/gnostic-models#8 . But receives no response for over 1 year. Maybe we can try again

@Jefftree
Copy link
Member

can we request a tag on the gnostic-models repo so we don't have to switch from a tag to an untagged commit?

ack, I'll work on getting a new version released and probably also bumping some of the extremely outdated packages.

@Jefftree
Copy link
Member

/hold

until google/gnostic-models#8 is resolved

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2024
@Jefftree
Copy link
Member

/hold cancel
Published v0.6.9 for gnostic-models: https://github.com/google/gnostic-models/releases/tag/v0.6.9.

@huww98 can you update this PR to point to the new tag?

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 26, 2024
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 27, 2024
@huww98
Copy link
Contributor Author

huww98 commented Nov 27, 2024

can you update this PR to point to the new tag?

Done

liggitt
liggitt reviewed Nov 27, 2024
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
liggitt
liggitt reviewed Nov 27, 2024
View reviewed changes
.github/workflows/ci.yml Outdated
@@ -34,10 +34,10 @@ jobs:
go test ./...
# We set the maximum version of the go directive as 1.20 here
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# We set the maximum version of the go directive as 1.20 here
# We set the maximum version of the go directive as 1.21 here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Sorry. Maybe we should just remove these redundant information. To make future updates easier.

@huww98
remove old github.com/golang/protobuf
36bdabb 
gnostic-models has not been updated for over 1 year. Just use its latest main branch, as in gnostic repo
@huww98 huww98 mentioned this pull request Nov 27, 2024
Merged
Jefftree
Jefftree reviewed Nov 27, 2024
View reviewed changes
go.mod Show resolved Hide resolved
@liggitt
Copy link
Member

liggitt commented Nov 27, 2024

lgtm

@Jefftree
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: huww98, Jefftree

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2024
@k8s-ci-robot k8s-ci-robot merged commit 9959940 into kubernetes:master Nov 27, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jefftree Jefftree Jefftree left review comments

@liggitt liggitt liggitt left review comments

@apelisse apelisse Awaiting requested review from apelisse

@roycaihw roycaihw Awaiting requested review from roycaihw

Assignees

@Jefftree Jefftree

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@huww98 @Jefftree @aojea @liggitt @k8s-ci-robot