-
Notifications
You must be signed in to change notification settings - Fork 587
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
Add sources metadata conformance test #3319
Conversation
Skipping CI for Draft Pull Request. |
There was a problem hiding this 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 -type f -name '*.go' -print)
There was a problem hiding this 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 -type f -name '*.go' -print)
There was a problem hiding this 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 -type f -name '*.go' -print)
/test all |
@@ -39,7 +39,7 @@ import ( | |||
func BrokerV1Beta1ControlPlaneTestHelperWithChannelTestRunner( | |||
t *testing.T, | |||
brokerClass string, | |||
channelTestRunner lib.ChannelTestRunner, | |||
channelTestRunner lib.ComponentsTestRunner, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanna change this great (IMO) rename here as well:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, will do
@@ -25,11 +25,16 @@ import ( | |||
"knative.dev/eventing/test/lib" | |||
) | |||
|
|||
var channelLabels = map[string]string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
test/conformance/helpers/metadata.go
Outdated
"knative.dev/eventing/test/lib" | ||
) | ||
|
||
func objectHasRequiredLabels(client *lib.Client, object metav1.TypeMeta, key string, value string) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
objectHasRequiredLabel
? singular?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
// Each source MUST have the following: | ||
// label of duck.knative.dev/source: "true" | ||
t.Run("Source CRD has required label", func(t *testing.T) { | ||
yes, err := objectHasRequiredLabels(client, source, "duck.knative.dev/source", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps might move the duck.knative.dev/source
to a map as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this 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 -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v wire_gen.go)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moar conformance, noice!
test/e2e_flags.go
Outdated
SourceUsage = "The names of the source type metas, separated by comma. " + | ||
"Example: \"sources.knative.dev/v1alpha1:ApiServerSource," + | ||
"sources.knative.dev/v1alpha1:PingSource\"." | ||
) | ||
// EventingFlags holds the command line flags specific to knative/eventing. | ||
//var EventingFlags *EventingEnvironmentFlags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is a leftover and should be removed.
"messaging.knative.dev/subscribable": "true", | ||
"duck.knative.dev/addressable": "true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have constants for those labels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none that I know of, lemme know if you know them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't found any so I created:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscribable is here:
pkg/apis/messaging/register.go
https://github.com/knative/eventing/blob/master/pkg/apis/messaging/register.go#L25
// InitializeEventingFlags registers flags used by e2e tests, calling flag.Parse() here would fail in | ||
// go1.13+, see https://github.com/knative/test-infra/issues/1329 for details | ||
func InitializeEventingFlags() { | ||
flag.Var(&EventingFlags.Channels, "channels", "The names of the channel type metas, separated by comma. Example: \"messaging.knative.dev/v1alpha1:InMemoryChannel,messaging.cloud.google.com/v1alpha1:Channel,messaging.knative.dev/v1alpha1:KafkaChannel\".") | ||
flag.StringVar(&EventingFlags.BrokerClass, "brokerclass", "MTChannelBasedBroker", "Which brokerclass to test, requires the proper Broker implementation to have been installed, and only one value. brokerclass must be (for now) 'MTChannelBasedBroker'.") | ||
|
||
flag.Var(&EventingFlags.Channels, "channels", ChannelUsage) | ||
flag.StringVar(&EventingFlags.BrokerClass, "brokerclass", "MTChannelBasedBroker", BrokerUsage) | ||
flag.Var(&EventingFlags.Sources, "sources", SourceUsage) | ||
flag.Parse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling flag.Parse() here would fail
flag.Parse()
More seriously, the comment is probably outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've no context of this whole comment so I'd like to keep it out of this PR scope. I'll follow up on slack on it though since I've came across it. Good catch 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #3322
test/flags/channels.go
Outdated
return fmt.Sprint(*channels) | ||
} | ||
|
||
// Set converts the input string to Channels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Appends a new Channel corresponding to the input string" seems to describe the actual code better.
test/flags/sources.go
Outdated
return fmt.Sprint(*sources) | ||
} | ||
|
||
// Set converts the input string to Sources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Appends a new Source corresponding to the input string" seems to describe the actual code better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor this in a separate PR and actually rename the method to Append
. Good catch 👍 . I'm not fully familiar with the code base so I'm not sure who's using who and I wanna limit the blast radius of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we at least have an accurate comment in this PR?
test/flags/sources.go
Outdated
Kind: split[1], | ||
} | ||
if !isValidSource(tm.Kind) { | ||
log.Fatalf("The given source name %q is invalid, tests cannot be run.\n", source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed case within the function "Source/source"
test/flags/channels.go
Outdated
Kind: split[1], | ||
} | ||
if !isValidChannel(tm.Kind) { | ||
log.Fatalf("The given channel name %q is invalid, tests cannot be run.\n", channel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixed case within the function "Channel/channel"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Generally I think the error messages in the test folder can use some love
/lgtm |
@@ -37,7 +37,7 @@ var MessagingChannelTypeMeta = metav1.TypeMeta{ | |||
Kind: resources.ChannelKind, | |||
} | |||
|
|||
// ChannelFeatureMap saves the channel-features mapping. | |||
// ComponentFeatureMap saves the channel-features mapping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Abd4llA this one remains
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Abd4llA The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Holding until refactoring is done in eventing-contrib |
@Abd4llA: PR needs rebase. 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. |
/lgtm cancel |
How about we split this work into two PRs ?
While PR 1 was merged, the same changes could already be applied to |
Fixes #3116
Proposed Changes
Release Note