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

Types for v1 messaging #3403

Merged
merged 7 commits into from
Jun 25, 2020
Merged

Conversation

nlopezgi
Copy link
Contributor

@nlopezgi nlopezgi commented Jun 24, 2020

Part of #3336

Proposed Changes

  • Adds types for V1 messaging/*
  • NO v1beta conversions in this PR

This PR does not include the v1beta -> v1 conversions. Those have been delayed to an upcoming PR.

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 24, 2020
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Jun 24, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2020
@aliok
Copy link
Member

aliok commented Jun 25, 2020

@nlopezgi I can help with conversion and possibly others but that means this PR should just bump the types and gets merged.
Let me know ;)

@lionelvillard
Copy link
Member

@nlopezgi I agree with @aliok. This PR should just be about types, nothing else. From my PoV I can then work on Flows v1.

@matzew
Copy link
Member

matzew commented Jun 25, 2020

+1 @lionelvillard

let's keep this small and tiny - much better to tackle

Copy link
Member

@mattmoor mattmoor left a 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)

pkg/apis/messaging/v1beta1/channel_conversion_test.go Outdated Show resolved Hide resolved
pkg/apis/messaging/v1beta1/channel_conversion_test.go Outdated Show resolved Hide resolved
@nlopezgi
Copy link
Contributor Author

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.

@nlopezgi nlopezgi changed the title [WIP]: Types for v1 messaging Types for v1 messaging Jun 25, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 25, 2020
@nlopezgi
Copy link
Contributor Author

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.

@aliok
Copy link
Member

aliok commented Jun 25, 2020

@nlopezgi now it is the end of the day for me. I can do the subscription conversion tomorrow though, no worries.

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/messaging/v1/channel_conversion.go Do not exist 100.0%
pkg/apis/messaging/v1/channel_defaults.go Do not exist 100.0%
pkg/apis/messaging/v1/channel_lifecycle.go Do not exist 84.0%
pkg/apis/messaging/v1/channel_types.go Do not exist 100.0%
pkg/apis/messaging/v1/channel_validation.go Do not exist 95.8%
pkg/apis/messaging/v1/in_memory_channel_conversion.go Do not exist 100.0%
pkg/apis/messaging/v1/in_memory_channel_defaults.go Do not exist 100.0%
pkg/apis/messaging/v1/in_memory_channel_lifecycle.go Do not exist 75.9%
pkg/apis/messaging/v1/in_memory_channel_types.go Do not exist 100.0%
pkg/apis/messaging/v1/in_memory_channel_validation.go Do not exist 100.0%
pkg/apis/messaging/v1/register.go Do not exist 0.0%
pkg/apis/messaging/v1/subscribable_channelable_validation.go Do not exist 100.0%
pkg/apis/messaging/v1/subscription_conversion.go Do not exist 100.0%
pkg/apis/messaging/v1/subscription_defaults.go Do not exist 100.0%
pkg/apis/messaging/v1/subscription_lifecycle.go Do not exist 60.0%
pkg/apis/messaging/v1/subscription_types.go Do not exist 66.7%
pkg/apis/messaging/v1/subscription_validation.go Do not exist 93.3%

@knative-prow-robot
Copy link
Contributor

@nlopezgi: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-eventing-go-coverage 9cf3032 link /test pull-knative-eventing-go-coverage

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.

@nlopezgi
Copy link
Contributor Author

@matzew addressed comments and CI is passing. Let me know if anything else is needed here.

@lionelvillard
Copy link
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2020
@matzew
Copy link
Member

matzew commented Jun 25, 2020

/lgtm
/approve

@knative-prow-robot
Copy link
Contributor

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2020
@knative-prow-robot knative-prow-robot merged commit aafe4de into knative:master Jun 25, 2020
lberk pushed a commit to lberk/eventing that referenced this pull request Jun 26, 2020
* adding v1 API for messaging types

* fixes

* conversions stashing work

* channel conversion

* removing changes to conversion to make this PR smaller

* address review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants