-
Notifications
You must be signed in to change notification settings - Fork 38
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
Feature config #82
Feature config #82
Conversation
Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper: The label(s) 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: slinkydeveloper 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 |
Signed-off-by: Francesco Guardiani <[email protected]>
"sigs.k8s.io/yaml" | ||
) | ||
|
||
var globalConfig map[string][]byte |
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.
no globals please
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 wonder if this should part of the Environment
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.
How do you propose to implement it instead? Note that by design the config reading is one per process and I expect the user to invoke it in TestMain
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 wonder if this should part of the Environment
Sounds like a good idea
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.
Nono i'm sorry, just checked, it doesn't work using env, because a feature can be built without the env
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.
So I believe we should leave it as a global as is 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.
because a feature can be built without the env
But a feature is executed against an environment - and technically are we not 'configuring' the feature in order for the test to pass?
I feel like this is related to: #66
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.
technically are we not 'configuring' the feature in order for the test to pass?
But the configuration happens before the test runs
Signed-off-by: Francesco Guardiani <[email protected]>
var globalConfig map[string][]byte | ||
|
||
// ReadConfigNamed is like ReadConfig, but you can specify a file name | ||
func ReadConfigNamed(name 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.
instead of the global, you can return the parsed config.
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.
That's not the way i thought to configuration. I would love to have a global conf for the whole test suite and then single "feature factories" read that config as they want.
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.
the plan was to have that but do it in the testing domain, like eventing or serving and not the testing framework
) | ||
|
||
func (f *Feature) Config(out interface{}) interface{} { | ||
err := config.UnmarshalConfig(f.Name, out) |
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.
just name might not be enough to know which config to load. For example, "BrokerTest" is likely going to be something that is used a few times?
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.
My expectation is that test names are unique. I'm thinking about a "nested" configuration, but as a next step
I do wonder how this would work with features that vendor and extend. Which leads me to think the config should always be passed to the feature constructor and not as the example shows where the feature constructor pulls out of global config. |
My expectation is that the user can configure per feature the various parameters, eg in this test https://github.com/knative/eventing/blob/master/test/rekt/features/broker_feature.go#L37 one more than passing the parameters, just gets the conf, corresponding to the feature name, and parse it to read the broker class |
Ok I feel like I misunderstood how we expect users to configure features:
|
I was expecting feature constructors to take in args when the test they do can be or wants to be parameterized. I think the author of the test needs to consider how the test is extended and vendored. |
If that's the case, then this PR doesn't make any sense. I'll close it |
Fix #31
Signed-off-by: Francesco Guardiani [email protected]
Changes
Fixes #
Release Note