Change how plugins work #133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
201711071651_sonobuoy_d2e178db-0eeb-4c35-9001-f3d84be72fc2.tar.gz Here is a sample run using this code. |
timothysc
suggested changes
Nov 7, 2017
| if err != nil { | ||
| var b bytes.Buffer | ||
| p.Definition.Template.Execute(&b, p.DfnTemplateData) | ||
| var daemonSet v1beta1ext.DaemonSet |
There was a problem hiding this comment.
Please group the var declarations for consistency.
I really wish the type could be inferred and plugins be more
| return errors.Wrapf(err, "could not create ConfigMap for daemonset plugin %v", p.GetName()) | ||
| } | ||
| if _, err = kubeclient.ExtensionsV1beta1().DaemonSets(p.Namespace).Create(daemonSet); err != nil { | ||
| if _, err := kubeclient.ExtensionsV1beta1().DaemonSets(p.DfnTemplateData.Namespace).Create(&daemonSet); err != nil { |
There was a problem hiding this comment.
Add a // TODO to switch to .Apps in once extensions has been deprecated, or we will need to explicitly wrap.
|
|
||
| // Delete the ConfigMap created by this plugin | ||
| err = kubeclient.CoreV1().ConfigMaps(p.Namespace).DeleteCollection( | ||
| err := kubeclient.ExtensionsV1beta1().DaemonSets(p.DfnTemplateData.Namespace).DeleteCollection( |
There was a problem hiding this comment.
so the if err != nil handling here is inconsistent. Let follow the boy-scout model and make the campground a bit cleaner then when we arrived.
pkg/plugin/driver/job/job.go
Outdated
| if err != nil { | ||
| var b bytes.Buffer | ||
| p.Definition.Template.Execute(&b, p.DfnTemplateData) | ||
| var job v1.Pod |
pkg/plugin/loader/loader.go
Outdated
| cfg := &plugin.WorkerConfig{ | ||
| ResultType: dfn.ResultType, | ||
| } | ||
| // TODO(cha): could just pass `masterAddress` directly into the NewPlugin functions. |
pkg/plugin/loader/loader.go
Outdated
| if err != nil { | ||
| logrus.Warningf("Error unmarshalling bytes at %v: %v", fullPath, err) | ||
| continue | ||
| return pluginDfns, err |
There was a problem hiding this comment.
why not return nil,err on failure here?
There was a problem hiding this comment.
I realize it's nil, but it's less explicit when reading.
timothysc
reviewed
Nov 7, 2017
examples/quickstart.yaml
Outdated
| @@ -261,7 +290,7 @@ spec: | |||
| valueFrom: | |||
| fieldRef: | |||
| fieldPath: status.podIP | |||
| image: gcr.io/heptio-images/sonobuoy:master | |||
| image: gcr.io/heptio-images/sonobuoy:latest | |||
timothysc
reviewed
Nov 7, 2017
| @@ -47,6 +47,7 @@ func LoadConfig() (*plugin.WorkerConfig, error) { | |||
| viper.BindEnv("masterurl", "MASTER_URL") | |||
| viper.BindEnv("nodename", "NODE_NAME") | |||
| viper.BindEnv("resultsdir", "RESULTS_DIR") | |||
| viper.BindEnv("resulttype", "RESULT_TYPE") | |||
There was a problem hiding this comment.
We do need this. I am providing these values to the worker via Env Vars in the plugin template. This allows me to bypass the ConfigMap entirely.
Instead of creating our own schema for a plugin we will use annotations to get metadata to sonobuoy instead. We can then define full kubernetes resources as plugins. The resources we define are templates. The idea is to eventually move these templates over to ksonnet and have a nice ksonnet library for the shared pieces between plugins. Remaining work: * Whatever configuration can be passed into the template should be allowed to be passed into the template (`RESULTS_DIR`). * Add a test or two to the plugin.Interface implementations Signed-off-by: Chuck Ha <[email protected]>
jhamilton1
pushed a commit
to jhamilton1/sonobuoy
that referenced
this pull request
Jun 27, 2018
Change how plugins work Signed-off-by: Jesse Hamilton [email protected]
jhamilton1
pushed a commit
to jhamilton1/sonobuoy
that referenced
this pull request
Jun 27, 2018
Change how plugins work Signed-off-by: Jesse Hamilton [email protected] Signed-off-by: Jesse Hamilton [email protected]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of creating our own schema for a plugin we will use annotations
to get metadata to sonobuoy instead. We can then define full kubernetes
resources as plugins.
The resources we define are templates. The idea is to eventually move
these templates over to ksonnet and have a nice ksonnet library for the
shared pieces between plugins.
Remaining work:
should be allowed to be passed into the template (
RESULTS_DIR).Fixes: #96
Signed-off-by: Chuck Ha [email protected]