Fixes duplicate runs #401

chuckha · 2018-03-28T16:28:45Z

Adds lots of tests for merging behavior
Adds tests to some other parts of the code relating to configuration manipulation.
Removes duplicate search paths in the default case.

Signed-off-by: Chuck Ha chuck@heptio.com

What this PR does / why we need it:

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer:

Release note:

release-note

chuckha · 2018-03-28T16:29:59Z

pkg/client/gen.go

@@ -46,8 +43,6 @@ type templateValues struct {

 // GenerateManifest fills in a template with a Sonobuoy config
 func (c *SonobuoyClient) GenerateManifest(cfg *GenConfig) ([]byte, error) {
- // Provide defaults but don't overwrite any customized configuration.


I felt this was not the appropriate place for setting defaults

chuckha · 2018-03-28T17:04:51Z

Mar 28 17:04:08.301: INFO: Waiting up to 5m0s for pod "pod-secrets-074ad31f-32aa-11e8-81cd-46e2536abc14" in namespace "e2e-tests-secrets-gp6cv" to be "success or failure"
Mar 28 17:04:08.306: INFO: Pod "pod-secrets-074ad31f-32aa-11e8-81cd-46e2536abc14": Phase="Pending", Reason="", readiness=false. Elapsed: 5.52174ms
namespace="heptio-sonobuoy" pod="sonobuoy" container="kube-sonobuoy"
time="2018-03-28T17:04:05Z" level=info msg="Scanning plugins in ./plugins.d (pwd: /)"
time="2018-03-28T17:04:05Z" level=info msg="Scanning plugins in /etc/sonobuoy/plugins.d (pwd: /)"
time="2018-03-28T17:04:05Z" level=info msg="Directory (/etc/sonobuoy/plugins.d) does not exist"
time="2018-03-28T17:04:05Z" level=info msg="Scanning plugins in ~/sonobuoy/plugins.d (pwd: /)"
time="2018-03-28T17:04:05Z" level=info msg="Directory (~/sonobuoy/plugins.d) does not exist"
time="2018-03-28T17:04:05Z" level=info msg="Filtering namespaces based on the following regex:.*|heptio-sonobuoy"
time="2018-03-28T17:04:05Z" level=info msg="Namespace default Matched=true"
time="2018-03-28T17:04:05Z" level=info msg="Namespace e2e-tests-sched-pred-xg82s Matched=true"
time="2018-03-28T17:04:05Z" level=info msg="Namespace heptio-sonobuoy Matched=true"
time="2018-03-28T17:04:05Z" level=info msg="Namespace kube-public Matched=true"
time="2018-03-28T17:04:05Z" level=info msg="Namespace kube-system Matched=true"
time="2018-03-28T17:04:05Z" level=info msg="Starting server Expected Results: [{ e2e} {ip-10-0-10-220.us-west-2.compute.internal systemd_logs} {ip-10-0-23-198.us-west-2.compute.internal systemd_logs} {ip-10-0-24-246.us-west-2.compute.internal systemd_logs}]"
time="2018-03-28T17:04:05Z" level=info msg="Running plugin" plugin=e2e
time="2018-03-28T17:04:05Z" level=info msg="starting aggregation server" address=0.0.0.0 port=8080
time="2018-03-28T17:04:06Z" level=info msg="Running plugin" plugin=systemd-logs
time="2018-03-28T17:04:08Z" level=info msg="received aggregator request" client_cert=systemd-logs node=ip-10-0-10-220.us-west-2.compute.internal plugin_name=systemd_logs
namespace="heptio-sonobuoy" pod="sonobuoy-e2e-job-e64ff3a13dfc4beb" container="sonobuoy-worker"
time="2018-03-28T17:04:08Z" level=info msg="Waiting for waitfile" waitfile=/tmp/results/done

some logs to show it's working

liztio

only thing blocking is the giant thunk of duplicated tests.

liztio · 2018-03-28T17:40:14Z

cmd/sonobuoy/app/sonobuoyconfig_test.go

+ },
+ },
+ },
+ raw: "not empty",


what's this do?

This avoids the logic here https://github.com/heptio/sonobuoy/blob/master/cmd/sonobuoy/app/sonobuoyconfig.go#L63 so that I can supply my own config.

liztio · 2018-03-28T18:05:03Z

pkg/config/config.go

+
+ // TODO(chuckha) here is a safety net in case a supplied plugin search path is provided in the custom config.
+ // mergo will append the defaults to the provided slice which may not be the behavior we want.
+ // This could go away if we change the assumptions about loading plugins change.


could you be a little clearer about what the TODO is?

liztio · 2018-03-28T18:05:47Z

pkg/config/config.go

+ // This could go away if we change the assumptions about loading plugins change.
+ uniqueSearchPaths := make(map[string]struct{})
+ for _, searchPath := range cfg.PluginSearchPath {
+ if _, ok := uniqueSearchPaths[searchPath]; !ok {


why not just unconditionally insert the element? should be an effective no-op if you're inserting the same thing

also bool might make it more obvious you're just using this as a ersatz set

empty struct is convention for a map used as a set, at least from what i've seen. I think kubernetes straight up maps type Empty struct{} for the sets it uses. Either way this code is going away

liztio · 2018-03-28T18:06:32Z

pkg/config/config_test.go

@@ -14,16 +14,19 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-package config
+package config_test


any particular reason for this?

This test only tested the public api and I've found that when tests are in the same package there is a tendency to test internal details that don't really need testing. If for some reason a package local function needs testing and testing the public api that uses it isn't sufficient we can add another test file later if that sounds ok to you.

liztio · 2018-03-28T18:11:07Z

pkg/config/config_test.go

@@ -37,3 +40,96 @@ func TestDefaults(t *testing.T) {
 t.Fatalf("Defaults should match but didn't")
 }
 }
+
+func TestMerge(t *testing.T) {


these tests seem pretty much identical to the ones above. Are they exercising something significantly different?

true, i'll clean these up

chuckha · 2018-03-28T19:38:02Z

pkg/plugin/loader/loader_test.go

+ plugin.Selection{Name: "daemonset"},
+ plugin.Selection{Name: "job"},
+ }
+ plugins, err := LoadAllPlugins(namespace, sonobuoyImage, imagePullPolicy, searchPath, selections)


whoops this test is broken. it passes but only because no tests are run. Fixing.

liztio

one minor nit, then lgtm!

liztio · 2018-03-28T19:33:50Z

pkg/plugin/loader/loader_test.go

@@ -192,3 +192,24 @@ func TestFilterList(t *testing.T) {
 t.Errorf("expected %+#v, got %+#v", expected, filtered)
 }
 }
+
+func TestLoadAllPlugins(t *testing.T) {


could you add a comment about what this is actually testing? or call it TestLoadAllPluginsRemovesDuplicates or smth

turned it into a table test with a description

* Ignore duplicate plugins during plugin loading. * Adds lots of tests for merging behavior. * Adds tests to some other parts of the code relating to configuration manipulation. Signed-off-by: Chuck Ha <chuck@heptio.com>

* Ignore duplicate plugins during plugin loading. * Adds lots of tests for merging behavior. * Adds tests to some other parts of the code relating to configuration manipulation. Signed-off-by: Chuck Ha <chuck@heptio.com> Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com

* Ignore duplicate plugins during plugin loading. * Adds lots of tests for merging behavior. * Adds tests to some other parts of the code relating to configuration manipulation. Signed-off-by: Chuck Ha <chuck@heptio.com> Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com

* Ignore duplicate plugins during plugin loading. * Adds lots of tests for merging behavior. * Adds tests to some other parts of the code relating to configuration manipulation. Signed-off-by: Chuck Ha <chuck@heptio.com>

chuckha requested a review from liztio March 28, 2018 16:28

chuckha commented Mar 28, 2018

View reviewed changes

liztio suggested changes Mar 28, 2018

View reviewed changes

chuckha force-pushed the 399 branch 2 times, most recently from 9d42626 to fe22a9d Compare March 28, 2018 19:30

chuckha commented Mar 28, 2018

View reviewed changes

chuckha force-pushed the 399 branch from fe22a9d to 20961f9 Compare March 28, 2018 19:41

liztio approved these changes Mar 28, 2018

View reviewed changes

Fixes duplicate runs

d539177

* Ignore duplicate plugins during plugin loading. * Adds lots of tests for merging behavior. * Adds tests to some other parts of the code relating to configuration manipulation. Signed-off-by: Chuck Ha <chuck@heptio.com>

chuckha force-pushed the 399 branch from 20961f9 to d539177 Compare March 28, 2018 20:21

chuckha merged commit bfaccaa into vmware-tanzu:master Mar 28, 2018

chuckha deleted the 399 branch March 28, 2018 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes duplicate runs #401

Fixes duplicate runs #401

chuckha commented Mar 28, 2018

chuckha Mar 28, 2018

chuckha commented Mar 28, 2018

liztio left a comment

liztio Mar 28, 2018

chuckha Mar 28, 2018

liztio Mar 28, 2018

liztio Mar 28, 2018

liztio Mar 28, 2018

chuckha Mar 28, 2018

liztio Mar 28, 2018

liztio Mar 28, 2018

chuckha Mar 28, 2018

liztio Mar 28, 2018

chuckha Mar 28, 2018

chuckha Mar 28, 2018

liztio left a comment

liztio Mar 28, 2018

chuckha Mar 28, 2018

Fixes duplicate runs #401

Fixes duplicate runs #401

Conversation

chuckha commented Mar 28, 2018

Choose a reason for hiding this comment

chuckha commented Mar 28, 2018

liztio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liztio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment