Feature config #82
Feature config #82
Conversation
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
|
@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 |
knative-prow-robot
added
the
size/L
| "sigs.k8s.io/yaml" | ||
| ) | ||
|
|
||
| var globalConfig map[string][]byte |
There was a problem hiding this comment.
I wonder if this should part of the Environment
There was a problem hiding this comment.
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.
I wonder if this should part of the Environment
Sounds like a good idea
There was a problem hiding this comment.
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.
So I believe we should leave it as a global as is now
There was a problem hiding this comment.
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.
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 <francescoguard@gmail.com>
| 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.
instead of the global, you can return the parsed config.
There was a problem hiding this comment.
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.
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.
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.
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 francescoguard@gmail.com
Changes
Fixes #
Release Note