-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
package config | ||
|
||
import ( | ||
"encoding/json" | ||
"errors" | ||
"fmt" | ||
"io/ioutil" | ||
"log" | ||
"os" | ||
"path" | ||
|
||
"sigs.k8s.io/yaml" | ||
) | ||
|
||
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 commentThe 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 commentThe 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 commentThe 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 |
||
// I expect the config in the main | ||
dir, err := os.Getwd() | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
configPath := path.Join(dir, name) | ||
|
||
log.Println("Reading config from ", configPath) | ||
|
||
out, err := ioutil.ReadFile(configPath) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
m := make(map[string]interface{}) | ||
err = yaml.Unmarshal(out, &m) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
globalConfig = make(map[string][]byte) | ||
for k, v := range m { | ||
b, err := json.Marshal(v) | ||
if err != nil { | ||
panic(err) | ||
} | ||
globalConfig[k] = b | ||
} | ||
} | ||
|
||
// ReadConfig reads the config | ||
func ReadConfig() { | ||
ReadConfigNamed("config.yaml") | ||
} | ||
|
||
// UnmarshalConfig reads the global config and unmarshal it | ||
func UnmarshalConfig(testName string, out interface{}) error { | ||
if globalConfig == nil { | ||
return errors.New("there's no global config!!! Make sure you invoke ReadConfig() or ReadConfigNamed(name) in your TestMain") | ||
} | ||
in := globalConfig[testName] | ||
if in == nil { | ||
return fmt.Errorf("there's no config for test name %s", testName) | ||
} | ||
return json.Unmarshal(in, out) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package feature | ||
|
||
import ( | ||
"fmt" | ||
|
||
"knative.dev/reconciler-test/pkg/config" | ||
) | ||
|
||
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 commentThe 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 commentThe 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 |
||
if err != nil { | ||
panic(fmt.Sprintf("cannot read config: %v", err)) | ||
} | ||
|
||
return out | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
RecorderFeature: | ||
Sink: my-sink | ||
Source: my-sender |
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.
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.
But the configuration happens before the test runs