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

Make reconcilers use messaging.* v1 resources #3584

Closed
aliok opened this issue Jul 14, 2020 · 15 comments
Closed

Make reconcilers use messaging.* v1 resources #3584

aliok opened this issue Jul 14, 2020 · 15 comments
Assignees
Labels
area/api kind/feature-request priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@aliok
Copy link
Member

aliok commented Jul 14, 2020

Part of #3446

Make reconcilers use messaging.* v1 resources

@aliok
Copy link
Member Author

aliok commented Jul 14, 2020

/assign

@aliok
Copy link
Member Author

aliok commented Jul 27, 2020

/unassign

This is a big task and I will be on PTO for a week. Unassigning from myself in case somebody wants to pick it up.

@lberk lberk added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. area/api labels Jul 27, 2020
@lberk lberk added this to the Backlog milestone Jul 27, 2020
@nlopezgi
Copy link
Contributor

nlopezgi commented Aug 4, 2020

/assign

@nlopezgi
Copy link
Contributor

nlopezgi commented Aug 6, 2020

The migration for channel and subscription might be blocked on a possible issue I have identified in #3789. Copying (with modifications) the comment I have there and bringing it here for better visibility:

I think there is some missing bits in the type migration from v1beta1 -> v1. Need confirmation from someone with more experience before I can proceed to make changes to the types.

Currently, the ChannelableCombined is a type that represents a skeleton type wrapping Subscribable and Addressable of both v1alpha1 and v1beta1 duck types.

I think that as part of the types migration we need to add something else there for the Subscribable and Addressable for v1, otherwise I'm not quite sure how to make a reconciler that can work with the ChannelableCombined properly.

Advice thoughts are much appreciated!

@n3wscott
Copy link
Contributor

n3wscott commented Aug 6, 2020

I think you will want to add the v1 shape to ChannelableCombined to make progress.

If there are missing fields we need to know asap, it will cause a huge storage headache for us in the next release with v1 going out. There should be round trip tests to catch this. v1 -> v1beta1 -> v1 should be lossless.

@nlopezgi
Copy link
Contributor

nlopezgi commented Aug 7, 2020

I think you will want to add the v1 shape to ChannelableCombined to make progress.

If there are missing fields we need to know asap, it will cause a huge storage headache for us in the next release with v1 going out. There should be round trip tests to catch this. v1 -> v1beta1 -> v1 should be lossless.

Thanks for confirming. I'll prepare a PR to add the v1 shape to ChannelableCombined

@nlopezgi
Copy link
Contributor

nlopezgi commented Aug 7, 2020

I think you will want to add the v1 shape to ChannelableCombined to make progress.

If there are missing fields we need to know asap, it will cause a huge storage headache for us in the next release with v1 going out. There should be round trip tests to catch this. v1 -> v1beta1 -> v1 should be lossless.

Created #3802 to track fixing this issue.

@aliok
Copy link
Member Author

aliok commented Aug 25, 2020

/close

This is done

@knative-prow-robot
Copy link
Contributor

@aliok: Closing this issue.

In response to this:

/close

This is done

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.

@nlopezgi
Copy link
Contributor

I think Subscription is still missing: https://github.com/knative/eventing/tree/master/pkg/reconciler/subscription. I can probably work on it if no one else is currently working on it

@aliok
Copy link
Member Author

aliok commented Aug 25, 2020

@nlopezgi

oh sorry, I thought that was done too.

I can probably work on it if no one else is currently working on it
Please take it over. Thanks!

Feel free to reopen this ticket or create a new one.

@nlopezgi
Copy link
Contributor

/reopen

@knative-prow-robot
Copy link
Contributor

@nlopezgi: Reopened this issue.

In response to this:

/reopen

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.

@nlopezgi
Copy link
Contributor

/close

@knative-prow-robot
Copy link
Contributor

@nlopezgi: Closing this issue.

In response to this:

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api kind/feature-request priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

No branches or pull requests

5 participants