-
Notifications
You must be signed in to change notification settings - Fork 592
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
Types for v1 messaging #3403
Types for v1 messaging #3403
Conversation
@nlopezgi I can help with conversion and possibly others but that means this PR should just bump the types and gets merged. |
let's keep this small and tiny - much better to tackle |
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.
Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)
Sorry about my delay getting back, will get this PR to just be about types and remove all the conversion bits, I dont know why I assumed all these bits needed to be together in the same PR. |
Ok, so iiuc this should be now ready for review (once CI passes). ptal @matzew @lionelvillard @aliok thanks for volunteering to help out with conversions, it is turning to be a bit more work that I wanted to. I've already almost completed the channel conversion for v1beta1. If you want to take on the imc_channel or subscription conversion that would be helpful. Let me know which one you want to work on so we don't duplicate work. It might make more sense for me to do imc_channel that is likely similar to channel, so if you want to take on doing the subsription_conversion that would be great. Please let me know if so. |
@nlopezgi now it is the end of the day for me. I can do the subscription conversion tomorrow though, no worries. |
@nlopezgi: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@matzew addressed comments and CI is passing. Let me know if anything else is needed here. |
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew, nlopezgi 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 |
* adding v1 API for messaging types * fixes * conversions stashing work * channel conversion * removing changes to conversion to make this PR smaller * address review comments
Part of #3336
Proposed Changes
This PR does not include the v1beta -> v1 conversions. Those have been delayed to an upcoming PR.