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

Write plugin definitions in their respective folders #812

Merged
merged 1 commit into from
Jul 24, 2019
Merged

Write plugin definitions in their respective folders #812

merged 1 commit into from
Jul 24, 2019

Conversation

johnSchnake
Copy link
Contributor

@johnSchnake johnSchnake commented Jul 24, 2019

What this PR does / why we need it:
The plugin info used to be written in the config but that was
removed and also didn't have any of the information like sessionID,
fields that could be altered by the config, etc.

This change dumps the loaded configuration data into the folder
at which users will most likely look for it.

In order to avoid an import cycle, it also removes a type that was
not used anywhere.

Separated this small bit of logic from my larger PR on making
plugin results more clearly reported.

Which issue(s) this PR fixes
Related to #306

Special notes for your reviewer:

Release note:

Adds a `definition.json` file for each plugin run in its respective `plugin/<name>` folder in the results tarball, making it easier to see what plugin information was used to produce the output and other bits of useful data like the session ID (useful if looking for resources or in logs) and whether or not the plugin cleaned up successfully.

@johnSchnake johnSchnake requested a review from zubron July 24, 2019 18:07
@codecov-io
Copy link

codecov-io commented Jul 24, 2019

Codecov Report

Merging #812 into master will decrease coverage by 0.07%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #812      +/-   ##
==========================================
- Coverage   43.42%   43.35%   -0.08%     
==========================================
  Files          71       71              
  Lines        4260     4274      +14     
==========================================
+ Hits         1850     1853       +3     
- Misses       2301     2314      +13     
+ Partials      109      107       -2
Impacted Files Coverage Δ
pkg/discovery/discovery.go 4.47% <11.11%> (-0.53%) ⬇️
cmd/sonobuoy/app/status.go 57.14% <0%> (-2.2%) ⬇️
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 f0ffecc...c59d0d5. Read the comment docs.

Copy link
Contributor

@zubron zubron left a comment

Choose a reason for hiding this comment

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

Thanks, @johnSchnake! I just have one question about the file permissions but otherwise it's good 👍

err = ioutil.WriteFile(
filepath.Join(outputDir, results.PluginsDir, p.GetName(), pluginDefinitionFilename),
b,
os.FileMode(0666),
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point, but would 644 be enough for the permissions here? That's the permissions that other files in the resulting tarball use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Thanks for pointing that out; I just chose a random value and thought I'd come back to it and forgot. That would be fine for me.

The plugin info used to be written in the config but that was
removed and also didn't have any of the information like sessionID,
fields that could be altered by the config, etc.

This change dumps the loaded configuration data into the folder
at which users will most likely look for it.

In order to avoid an import cycle, it also removes a type that was
not used anywhere.

Separated this small bit of logic from my larger PR on making
plugin results more clearly reported.

Related to #306

Signed-off-by: John Schnake <[email protected]>
@johnSchnake johnSchnake merged commit fe23f98 into vmware-tanzu:master Jul 24, 2019
@johnSchnake johnSchnake deleted the dumpPluginInfo branch July 24, 2019 20:05
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