Skip to content
This repository has been archived by the owner on Jun 19, 2022. It is now read-only.

Channel v1beta1 Fixes #959

Merged
merged 16 commits into from
May 8, 2020
Merged

Channel v1beta1 Fixes #959

merged 16 commits into from
May 8, 2020

Conversation

Harwayne
Copy link
Contributor

@Harwayne Harwayne commented Apr 28, 2020 •
edited
Loading

Should have been included in #615.

Proposed Changes

  • Inline spec.SubscribableSpec, as required by the duck v1beta1 Subscribable contract.
    • Alter the OpenAPIV3Schema to match.
  • Add the messaging.knative.dev/subscribable annotation to Channel. This allows the correct creation of Subscriptions on v1beta1 Channels.
  • Add v1beta1 Channels to the E2E tests.

Release Note

v1beta1 Channels remove a layer of nesting:
`spec.subscribable.subscribers` -> `spec.subscribers`

`Subscription`s can be made against v1beta1 `Channel`s.

@Harwayne
Add the messaging.knative.dev/subscribable annotation to Channel.
48d3540 
This allows the correct creation of Subscriptions on the Channels.
@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Apr 28, 2020
@Harwayne
Copy link
Contributor Author

/retest

grantr
grantr approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold
Holding for the answer to the question below. Cancel the hold if no changes are required.

pkg/apis/messaging/internal/constants.go
package internal

// StoredChannelVersion is the version of the Channel that is stored in the API server.
const StoredChannelVersion = "v1alpha1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this is the stored version of the PubSub Channel CRD, right? Not the stored version of the Channel CRD from Knative Eventing?

How will we ensure that when the stored version changes, this constant is also updated? Do we need a comment in the CRD yaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the version of the PubSub channel stored in the API server. Added a comment to the YAML.

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, Harwayne

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

@Harwayne
Copy link
Contributor Author

Harwayne commented May 1, 2020

/hold

Pulling at a thread caused a lot of changes. Holding until I have updated the title and description.

@Harwayne Harwayne changed the title Add the messaging.knative.dev/subscribable annotation to Channel. [WIP] Add the messaging.knative.dev/subscribable annotation to Channel. May 1, 2020
@Harwayne Harwayne changed the title [WIP] Add the messaging.knative.dev/subscribable annotation to Channel. [WIP] Channel v1beta1 Fixes May 1, 2020
@Harwayne
Copy link
Contributor Author

Harwayne commented May 1, 2020

/retest

2 similar comments
@Harwayne
Copy link
Contributor Author

Harwayne commented May 1, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 1, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 4, 2020

/retest

2 similar comments
@Harwayne
Copy link
Contributor Author

Harwayne commented May 5, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 5, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 6, 2020

/retest

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/messaging/v1alpha1/channel_conversion.go Do not exist 93.9%

@Harwayne
Copy link
Contributor Author

Harwayne commented May 7, 2020

/retest

2 similar comments
@Harwayne
Copy link
Contributor Author

Harwayne commented May 7, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 7, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented May 7, 2020

/retest

I swear this is working on my local machine...

@Harwayne
Copy link
Contributor Author

Harwayne commented May 8, 2020

/retest

@Harwayne Harwayne changed the title [WIP] Channel v1beta1 Fixes Channel v1beta1 Fixes May 8, 2020
@Harwayne
Copy link
Contributor Author

Harwayne commented May 8, 2020

/hold cancel

This is ready for review.

yolocs
yolocs reviewed May 8, 2020
View reviewed changes
Copy link
Member

@yolocs yolocs left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
Left a question. Feel free to unhold

pkg/apis/messaging/v1beta1/channel_defaults.go Show resolved Hide resolved
@Harwayne
Copy link
Contributor Author

Harwayne commented May 8, 2020

/unhold

I think I answered the question. If you want the comment expanded on, I'll do it in a follow up.

@knative-prow-robot knative-prow-robot merged commit daa9c7b into google:master May 8, 2020
@Harwayne Harwayne deleted the subscribable-version branch May 11, 2020 16:40
@knative-prow-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-google-knative-gcp-build-tests 4468c1f link /test pull-google-knative-gcp-build-tests
pull-google-knative-gcp-integration-tests 14bddba link /test pull-google-knative-gcp-integration-tests

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yolocs yolocs yolocs left review comments

@grantr grantr grantr approved these changes

@capri-xiyue capri-xiyue Awaiting requested review from capri-xiyue

Assignees

@grantr grantr

@yolocs yolocs

Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Harwayne @knative-prow-robot @knative-metrics-robot @grantr @yolocs @googlebot