Remove use of templates for plugins #834
Conversation
zubron
changed the title
cmd/sonobuoy/app/gen_plugin.go
Outdated
| @@ -1,127 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Should be able to rebase onto upstream/master and this commit shouldn't come up anymore.
| @@ -42,23 +45,6 @@ type Base struct { | |||
| CustomAnnotations map[string]string | |||
| } | |||
|
|
|||
| // TemplateData is all the fields available to plugin driver templates. | |||
| type TemplateData struct { | |||
There was a problem hiding this comment.
🎉 for datatypes we get to remove.
zubron
force-pushed
the
customise-pod-options-809
branch
from
ffef814 to
aab2214
Compare
Codecov Report
@@ Coverage Diff @@
## master #834 +/- ##
=========================================
+ Coverage 44.73% 46.9% +2.17%
=========================================
Files 75 75
Lines 4565 4667 +102
=========================================
+ Hits 2042 2189 +147
+ Misses 2390 2348 -42
+ Partials 133 130 -3
Continue to review full report at Codecov.
|
zubron
force-pushed
the
customise-pod-options-809
branch
from
3a2ea2e to
10e31d2
Compare
zubron
changed the title
pkg/plugin/constants.go
Outdated
| @@ -19,4 +19,7 @@ package plugin | |||
| const ( | |||
| // GracefulShutdownPeriod is how long plugins have to cleanly finish before they are terminated. | |||
| GracefulShutdownPeriod = 60 | |||
|
|
|||
| // ResultsDir is the diectory where results will be available in Sonobuoy plugin containers. | |||
pkg/plugin/driver/base_test.go
Outdated
| } | ||
| } | ||
| for _, e := range wc.Env { | ||
| if e.Name == "NODE_NAME" { |
There was a problem hiding this comment.
nit: would prefer a switch instead of an if...else...if else....if else...
There was a problem hiding this comment.
Sure, and thanks for the suggestion. I'm not particularly happy with this test :-/
There was a problem hiding this comment.
It is a little odd the way you have subtests but it isn't a table driven test.
The other options it seems:
- It is testing a single large object so you could dump it into one big test instead of bothering with the sub-tests. The error messages would just provide the context
- make a table driven test/subtests and split off the larger chunks of logic (which make it hard to read) into little helper funcs. So you'd have:
tc := testCases{
desc string
expectFunc func(<types we're checking>) error
}{
{ desc: "Env vars set",
expectFunc: checkEnvVars},
...
}
The benefit to (1) is that its straight forward to code but the downside it isn't clear when reading any particular section of the test what has been tested, will be tested, could be added, etc.
The benefit of (2) is that you can look at the table and atleast see a list of all the things you intend to test about it (even if only vague details). The downside is you have to reorg the code a bit to put those test functions in an accessible place and it may seem a bit odd since you won't be reusing those functions elsewhere. But you could, within that test itself, define those functions inline and then only make the table at the end:
func TestXYZ(t *testing.T){
checkEnvVars:= func(...) error{...}
checkFields:= func(...) error{...}
tc := testCases {...}{...}
...
I think I would probably prefer (2) if I were writing it, but I wont block it either way.
There was a problem hiding this comment.
I decided to change this test and do it the way you mentioned in (2). Let me know what you think.
zubron
force-pushed
the
customise-pod-options-809
branch
from
10e31d2 to
a9efcae
Compare
| }{ | ||
| { | ||
| "Fields set correctly", | ||
| checkFields, |
There was a problem hiding this comment.
👍 I do like reading the test more this way. It is also clear how I could/should expand the functionality if I needed to test something else.
This change removes the use of templates for creating resources when running plugins and instead creates the objects directly to use with the Kubernetes API. This change should introduce no change in behaviour. It is just refactoring in preparation for adding support to customise pod options. Signed-off-by: Bridget McErlean <[email protected]>
zubron
force-pushed
the
customise-pod-options-809
branch
from
a9efcae to
4c0bd38
Compare
What this PR does / why we need it:
This change removes the use of templates for creating resources when
running plugins and instead creates the objects directly to use with the
Kubernetes API. This change should introduce no change in behaviour. It
is just refactoring in preparation for adding support to customise pod
options.
Special notes for your reviewer:
I have added tests for the new changes in
base.go. I have only adapted the existing tests for checking that the manifests are created correctly. I can expand on these tests if needed. I have also tested this manually by comparing the resources created for both Job and DaemonSet plugins before and after this change and they are equivalent.Release note: