Skip to content
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

Remove plugin.Definition #810

Merged
merged 1 commit into from
Jul 23, 2019
Merged

Remove plugin.Definition #810

merged 1 commit into from
Jul 23, 2019

Conversation

johnSchnake
Copy link
Contributor

@johnSchnake johnSchnake commented Jul 23, 2019

What this PR does / why we need it:
The type plugin.Definition completely duplicates fields that exist
inside of manifest.Manifest and adds nothing new. Though I prefer
the name plugin.Definition, the users currently use the manifest.Manifest
object locally and expect the nested sonobuoy-config object so we
needed to keep that in order to not break existing plugins. In addition,
the plugin.Definition type was missing the Driver field, so the server
was actually losing that data when using that type.

This change should have no impact other than clarifying the landscape
of types on the backend.

Which issue(s) this PR fixes
Helps facilitate #306

Release note:

NONE

ResultType string `json:"result-type"`

// SkipCleanup informs Sonobuoy to leave the pods created for this plugin running,
// after the run completes instead of deleting them as part of default, cleanup behavior.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments here since I had started adding them on the plugin.Definition in my other branch related to #306

@@ -235,7 +235,7 @@ func TestFilterList(t *testing.T) {
{Name: "test3"},
}

expected := []*manifest.Manifest{definitions[0]}
expected := []manifest.Manifest{definitions[0]}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a bunch of situations, we were using pointers when I don't think it was necessary (and we had gone from a non-pointer plugin.Definition to a pointer to a manifest.Manifest).

@@ -62,16 +61,6 @@ type Interface interface {
SkipCleanup() bool
}

// Definition defines a plugin's features, method of launch, and other
// metadata about it.
type Definition struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main thing I just removed here; it is just a flattened version of the manifest.Manifest except it doesnt have the Driver field. It leads to us having an extra layer to transfer data through and doesn't provide any clear benefit.

@johnSchnake johnSchnake requested a review from zubron July 23, 2019 21:00
@johnSchnake johnSchnake marked this pull request as ready for review July 23, 2019 21:00
The type plugin.Definition completely duplicates fields that exist
inside of manifest.Manifest and adds nothing new. Though I prefer
the name plugin.Definition, the users currently use the manifest.Manifest
object locally and expect the nested sonobuoy-config object so we
needed to keep that in order to not break existing plugins. In addition,
the plugin.Definition type was missing the Driver field, so the server
was actually losing that data when using that type.

This change should have no impact other than clarifying the landscape
of types on the backend.

Helps facilitate #306

Signed-off-by: John Schnake <[email protected]>
@codecov-io
Copy link

codecov-io commented Jul 23, 2019

Codecov Report

Merging #810 into master will increase coverage by 0.03%.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #810      +/-   ##
==========================================
+ Coverage   43.24%   43.28%   +0.03%     
==========================================
  Files          71       71              
  Lines        4257     4251       -6     
==========================================
- Hits         1841     1840       -1     
+ Misses       2307     2304       -3     
+ Partials      109      107       -2
Impacted Files Coverage Δ
pkg/plugin/manifest/manifest.go 4% <ø> (ø) ⬆️
pkg/plugin/loader/loader.go 73.52% <100%> (-2.15%) ⬇️
pkg/plugin/driver/job/job.go 56.17% <25%> (ø) ⬆️
pkg/plugin/driver/base.go 42.3% <33.33%> (ø) ⬆️
pkg/plugin/driver/daemonset/daemonset.go 48.73% <60%> (ø) ⬆️
pkg/plugin/aggregation/aggregator.go 83.2% <0%> (+4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6240935...ff7829c. Read the comment docs.

@johnSchnake johnSchnake merged commit 020a7e4 into vmware-tanzu:master Jul 23, 2019
@johnSchnake johnSchnake deleted the pluginDefinition branch July 23, 2019 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants