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

Pubsub v1beta1 Conversions #784

Merged
merged 15 commits into from
Apr 10, 2020
Merged

Pubsub v1beta1 Conversions #784

merged 15 commits into from
Apr 10, 2020

Conversation

Harwayne
Copy link
Contributor

@Harwayne Harwayne commented Apr 7, 2020 •
edited
Loading

Helps with #615.

Proposed Changes

  • Adds the v1beta1 types PullSubscription and Topic to the API server.
  • Adds a conversion webhook that converts between v1alpha1 and v1beta1.

Release Note

PullSubscriptions and Topics now exist in both v1alpha1 and v1beta1 versions. The versions are almost identical. All existing objects will continue to work. All valid configurations for the objects will continue to be valid. 

If you wish, you may start using the v1beta1 API surface. If you choose not to, then everything will continue to work as-is.

Docs

@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Apr 7, 2020
@Harwayne
Make the OpenAPI schemna's structural.
d8d6097 
Give the webhook the required RBAC.
@Harwayne
Fix PullSubscription's OpenAPI schema by flusing out spec.secret and …
9d55a7a 
…preserving unknown fields where needed.
@Harwayne Harwayne mentioned this pull request Apr 8, 2020
Merged
@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 9, 2020

/retest

1 similar comment
@Harwayne
Copy link
Contributor Author

/retest

@Harwayne
Copy link
Contributor Author

/test pull-google-knative-gcp-wi-tests

1 similar comment
@Harwayne
Copy link
Contributor Author

/test pull-google-knative-gcp-wi-tests

@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/pubsub/v1alpha1/pullsubscription_conversion.go Do not exist 90.9%
pkg/apis/pubsub/v1alpha1/topic_conversion.go Do not exist 80.0%
pkg/apis/pubsub/v1beta1/pullsubscription_conversion.go Do not exist 100.0%
pkg/apis/pubsub/v1beta1/topic_conversion.go Do not exist 100.0%

@Harwayne
Copy link
Contributor Author

/test pull-google-knative-gcp-integration-tests

@Harwayne Harwayne marked this pull request as ready for review April 10, 2020 20:46
@Harwayne Harwayne changed the title [WIP] Pubsub v1beta1 Pubsub v1beta1 Conversions Apr 10, 2020
grantr
grantr approved these changes Apr 10, 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.

Thanks Adam! A lot of this looks like it could be a good candidate for a generator...

/lgtm
A couple comment nits, entirely optional for this PR.

cmd/webhook/main.go
},
},
},
func(ctx context.Context) context.Context {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: add a comment about the purpose of this argument, as in https://github.com/knative/eventing/blob/1e68f1cfe5bb4f8c67aca7b09b36b973cde278dc/cmd/webhook/main.go#L333

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#813

pkg/apis/pubsub/v1alpha1/pullsubscription_conversion_test.go
SubscriptionID: "subscriptionID",
}

// completePullSubscription is a PullSubscription with every filled in, except TypeMeta.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: every field filled in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#813

@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

@knative-prow-robot knative-prow-robot merged commit e361668 into google:master Apr 10, 2020
@Harwayne Harwayne deleted the pubsub-conversion branch April 10, 2020 23:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grantr grantr grantr approved these changes

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

Assignees

@grantr grantr

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

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