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

v1beta1 - Duck and Pubsub #779

Merged
merged 5 commits into from
Apr 10, 2020
Merged

v1beta1 - Duck and Pubsub #779

merged 5 commits into from
Apr 10, 2020

Conversation

Harwayne
Copy link
Contributor

@Harwayne Harwayne commented Apr 7, 2020 •
edited
Loading

Helps with #614

Proposed Changes

  • Create v1beta1 go types for:
    • Duck types
    • Pubsub types
  • This change does not actually alter what can be stored in the API server, nor does it change any code to use the new types.
  • All code is copied from the v1alpha1 types. There is only one change, in topic_lifecycle.go because the v1beta1 duck version of Addressable no longer has hostname, so its support was dropped.
    • This also caused a changed status condition when there is no addressable URL. It changed from emptyHostname to emptyUrl.

This was generated using:

foo=pubsub
cp  -R pkg/apis/$foo/v1alpha1/ pkg/apis/$foo/v1beta1
sed -i '.bak' 's/v1alpha1/v1beta1/g' pkg/apis/$foo/v1beta1/*
rm -f pkg/apis/$foo/v1beta1/*.bak

Release Note

NONE

@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Apr 7, 2020
@Harwayne Harwayne mentioned this pull request Apr 7, 2020
Merged
yolocs
yolocs reviewed Apr 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
In case you need to make change. This is just copying from v1alpha1 API right?

@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 8, 2020

/hold
In case you need to make change. This is just copying from v1alpha1 API right?

There was a single change, now mentioned in the PR description. Please take a look to make sure you are OK with it.

@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 8, 2020

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

yolocs
yolocs approved these changes Apr 9, 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

pkg/apis/pubsub/v1beta1/topic_lifecycle.go Outdated
topicCondSet.Manage(ts).MarkTrue(TopicConditionAddressable)
} else {
ts.Address.URL = nil
topicCondSet.Manage(ts).MarkFalse(TopicConditionAddressable, "emptyHostname", "hostname is the empty string")
Copy link
Member

Choose a reason for hiding this comment

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

nit: since hostname is gone, should reword the reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done. I changed both the v1alpha1 and v1beta1 branches, so that we don't suddenly change error messages when our reconcilers start using v1beta1.

yolocs
yolocs approved these changes Apr 9, 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

@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 9, 2020

/retest

grantr
grantr approved these changes Apr 9, 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

Don't think there's a need for the hold anymore, but @yolocs since the test just failed you have 30 minutes to object 😁

/hold cancel

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [Harwayne,grantr,yolocs]

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 Apr 9, 2020

/retest

@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 9, 2020

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

@Harwayne
Copy link
Contributor Author

Harwayne commented Apr 9, 2020

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

@Harwayne
Copy link
Contributor Author

/test pull-google-knative-gcp-go-coverage

@Harwayne
Copy link
Contributor Author

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

@yolocs
Copy link
Member

yolocs commented Apr 10, 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/duck/v1beta1/annotations.go Do not exist 93.2%
pkg/apis/duck/v1beta1/credentials.go Do not exist 100.0%
pkg/apis/duck/v1beta1/identity_lifecycle.go Do not exist 100.0%
pkg/apis/duck/v1beta1/identity_types.go Do not exist 100.0%
pkg/apis/duck/v1beta1/pubsub_defaults.go Do not exist 100.0%
pkg/apis/duck/v1beta1/pubsub_lifecycle.go Do not exist 0.0%
pkg/apis/duck/v1beta1/pubsub_types.go Do not exist 6.7%
pkg/apis/duck/v1beta1/register.go Do not exist 100.0%
pkg/apis/duck/v1beta1/resource_types.go Do not exist 25.0%
pkg/apis/pubsub/v1beta1/pullsubscription_defaults.go Do not exist 100.0%
pkg/apis/pubsub/v1beta1/pullsubscription_lifecycle.go Do not exist 100.0%
pkg/apis/pubsub/v1beta1/pullsubscription_types.go Do not exist 92.9%
pkg/apis/pubsub/v1beta1/pullsubscription_validation.go Do not exist 88.4%
pkg/apis/pubsub/v1beta1/register.go Do not exist 100.0%
pkg/apis/pubsub/v1beta1/topic_defaults.go Do not exist 100.0%
pkg/apis/pubsub/v1beta1/topic_lifecycle.go Do not exist 57.7%
pkg/apis/pubsub/v1beta1/topic_types.go Do not exist 100.0%
pkg/apis/pubsub/v1beta1/topic_validation.go Do not exist 100.0%

@Harwayne
Copy link
Contributor Author

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

2 similar comments
@Harwayne
Copy link
Contributor Author

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

@Harwayne
Copy link
Contributor Author

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

@Harwayne
Copy link
Contributor Author

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

1 similar comment
@Harwayne
Copy link
Contributor Author

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

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Apr 10, 2020 •
edited
Loading

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

Test name Commit Details Rerun command
pull-google-knative-gcp-go-coverage eece523 link /test pull-google-knative-gcp-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.

@Harwayne
Copy link
Contributor Author

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

@knative-prow-robot knative-prow-robot merged commit a9ab6f2 into google:master Apr 10, 2020
@Harwayne Harwayne deleted the v1beta1 branch April 10, 2020 20:10
@Harwayne Harwayne mentioned this pull request Apr 10, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grantr grantr grantr approved these changes

@yolocs yolocs yolocs approved these changes

Assignees

@grantr grantr

@yolocs yolocs

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.
6 participants
@Harwayne @knative-prow-robot @yolocs @knative-metrics-robot @grantr @googlebot