-
Notifications
You must be signed in to change notification settings - Fork 575
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
Do not parse flags in InitializeEventingFlags #6966
Conversation
Provides better flexibility for reusing in downstream repositories. It is now possible to initialize flags from this repo and any additional flags from downstream repositories and then call flag.Parse explicitly to fill the flags. Then it's possible to run RunMainTest independently. This is especially useful when we want to combine REKT tests with upgrade test suite.
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #6966 +/- ##
==========================================
- Coverage 79.98% 79.80% -0.19%
==========================================
Files 237 244 +7
Lines 12373 12793 +420
==========================================
+ Hits 9897 10209 +312
- Misses 1984 2078 +94
- Partials 492 506 +14 ☔ View full report in Codecov by Sentry. |
The flag doesn't exist anymore
@mgencur: once the present PR merges, I will cherry-pick it on top of release-1.10 in a new PR and assign it to you. In response to this:
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. |
@@ -43,17 +42,18 @@ const ( | |||
BrokerNameUsage = "When testing a pre-existing broker, specify the Broker name so the conformance tests " + | |||
"won't create their own." | |||
BrokerNamespaceUsage = "When testing a pre-existing broker, this variable specifies the namespace the broker can be found in." | |||
BrokerClass = "MTChannelBasedBroker" |
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.
Why not leaving BrokerClass
configurable?
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.
Because the only possible value is MTChannelBasedBroker. See the line which I removed: https://github.com/knative/eventing/pull/6966/files#diff-1bee0234a4a7bc66483f6aaffb29c6b0d6fc372bde980d3023c564fb30219893L77
Not sure if we'll have more options in the future. I can return it back but it's just redundant code right now.
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mgencur, pierDipi 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 |
@mgencur: new pull request created: #6971 In response to this:
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. |
Provides better flexibility for reusing in downstream repositories. It is now possible to initialize flags from this repo and any additional flags from downstream repositories and then call flag.Parse explicitly to fill the flags. Then it's possible to run RunMainTest independently.
This is especially useful when we want to combine REKT tests with upgrade test suite.
Fixes #
Proposed Changes
flag.Parse()
out of InitializeEventingFlags.Pre-review Checklist
Release Note
Docs