graph-builder: add plugin to parse openshift/cincinnati-graph-data #231

steveej · 2020-02-24T21:27:06Z

Add openshift_secondary_metadata_parser plugin:

Implement plugin logic
Test using fixtures produced with a custom graph-builder to obtain a
snapshot of the following triple:
- a vanilla graph produced by the registry scraper plugin
- a vanilla graph augmented by the quay secondary metadata plugin
- the data from the openshift/cincinnati-graph-data repository
  The test asserts equality of two results of the EdgeAddRemovePlugin.
  The first result is obtianed by passing the raw graph through the new
  parser plugin and then through the EdgeAddRemovePlugin.
  The second passing the quay metadata graph fixture through the new
  parser plugin.
  This ensures compatibility between the new plugin and the
  QuayMetadataPlugin with respect to equality for subsequent processing
  of their produced metadata.
Note: for this to work I had to add raw metadata to the fixtures,
which represents metadata present on quay, but not present as part of
the existing cincinnati-graph-data. This raw metadata has also been
added to the upstream data repository.

Split off the original #226.

Plugin to parse the contents of the repository and mangle the Graph
- Impl
- ~~Tests which rely on the previous plugin?~~
- Independent tests, i.e. generate a known file structure on-the-fly in the test
Double-check to either use tokio::fs functions or use spawn_blocking

wking · 2020-02-24T22:01:54Z

cincinnati/src/plugins/internal/edge_add_remove.rs

Do we need to make this tuneable? Can we just always do it?

It can be helpful for debugging to leave them in, so see what annotations are on the release. Also for now it's still off by default, which is compatible with the existing deployments.

Can this be used to force the plugin to discard previous processed metadata and start with fresh metadata scanning ?

This controls whether the metadata which is used by this plugin is removed after processing or not. As an example, if there is a previous.remove with the value 1.2.3 metadata on a release, this plugin will remove the edge, in the graph. If remove_consumed_metadata is true in the plugin configuration, it will also remove the metadata entry with the key previous.remove.

vrutkovs

Looks good, a few nitpicks and odd parts

vrutkovs · 2020-02-25T10:52:59Z

dist/prow_yaml_lint.sh

nitpick: define path as a var

there's only one usage of it though, and I could see us wanting to prune more than just this one. usually I'm all for constants if they benefit the DRY principle, but in this case it would just add indirection.

dist/prow_yaml_lint.sh

graph-builder/src/plugins/openshift_secondary_metadata_parser/plugin.rs

vrutkovs · 2020-02-25T11:02:53Z

graph-builder/src/plugins/openshift_secondary_metadata_parser/plugin.rs

Perhaps its worth sticking to yaml only until file extensions go out of control :)

This is currently in line with #226. I think both (yml and yaml) are common extensions. Do you think we should make it configurable?

They are common, but I'd rather not leave a space for errors here.

@wking @LalatenduMohanty do you agree we should stick to ".yaml" and ignore (possible) ".yml" in graph-data repo?

Standard practice for yaml file uses .yaml , .yml . So we should only support these two extensions. Just supporting yaml is fine too but then someone might request to add yml as well. So supporting .yaml , .yml makes us future proof.

vrutkovs · 2020-02-25T11:07:41Z

cincinnati/src/plugins/internal/edge_add_remove.rs

Seems both previous and previous_regex data would be applied. Shouldn't we prefer previous_regex instead?

I think the existing syntax can coexist with the additional one, so I don't see why we should remove the existing one. What makes you think we should?

I'd expect we use either previous or previous_regex in blocked-edges, but current implementation allows both. This may lead to a few odd situations - lets throw error when both are specified?

I don't see why it would be an error. They could be used in conjunction as well. Why do you want to restrict it?

Its not an error really, just feels that it might have undesirable consequences. Maybe I'm overreacting really as I can't come up with a potential issue on this one

If it helps, I don't see a potential issue in this part of the code either, and I've thought about it quite a bit 🙂

cincinnati/src/plugins/internal/edge_add_remove.rs

vrutkovs · 2020-02-25T11:19:32Z

graph-builder/src/plugins/openshift_secondary_metadata_parser/plugin.rs

I'm not entirely sure this would be a regex - is it an exact version?

If by this you mean blocked_edge.from, it is defined to be here: https://github.com/openshift/cincinnati/blob/89cd657bd552efdd1f8bd2c3c57a038e6cfecf9d/graph-builder/src/plugins/openshift_secondary_metadata_parser/plugin.rs#L20-L22

Oh, correct, thanks!

steveej · 2020-02-25T14:42:17Z

Looks good, a few nitpicks and odd parts

Thanks for the review! I addressed your comments either by code changes or answers.

steveej · 2020-02-25T18:17:36Z

/retest

steveej · 2020-02-26T09:44:25Z

Rebased and last TODO item completed!

steveej · 2020-02-26T11:22:47Z

/retest

LalatenduMohanty · 2020-02-26T14:13:09Z

@steveej Can we merge some of the commits with single line code changes still keep them logically separate? IMO all code changes related to the plugin should be just in one commit.

steveej · 2020-02-26T14:23:25Z

IMO all code changes related to the plugin should be just in one commit.

I suggest not squashing them, as it makes reverting commits more difficult in the future. Take ab718b4ab985601172e4345060990380693b2ba0 for example which exposes a constant. If this is squashed with the plugin commit, and we would want to revert the plugin commit, there is no guarantee the reversal won't break other code which in the meantime also depends on the public constant.

vrutkovs · 2020-02-26T15:49:28Z

It seems squashing 87a928f and ab718b4 would be a good idea.

Other than that it looks good

…Y_FILTER public

…adata removal setting * Add `previous.remove_regex` metadata key to remove edges by regex matching. * Add a `remove_consumed_metadata` configuration option to control the removal of metadata after it has been processed.

Add openshift_secondary_metadata_parser plugin: * Implement plugin logic * Test using fixtures produced with a custom graph-builder to obtain a snapshot of the following triple: * a vanilla graph produced by the registry scraper plugin * a vanilla graph augmented by the quay secondary metadata plugin * the data from the openshift/cincinnati-graph-data repository The test asserts equality of two results of the EdgeAddRemovePlugin. The first result is obtianed by passing the raw graph through the new parser plugin and then through the EdgeAddRemovePlugin. The second passing the quay metadata graph fixture through the new parser plugin. This ensures compatibility between the new plugin and the QuayMetadataPlugin with respect to equality for subsequent processing of their produced metadata. Note: for this to work I had to add raw metadata to the fixtures, which represents metadata present on quay, but not present as part of the existing cincinnati-graph-data. This raw metadata has also been added to the upstream data repository.

steveej · 2020-02-26T17:48:45Z

It seems squashing 87a928f and ab718b4 would be a good idea.

Agreed, these two are not unreasonable to squash, done!

vrutkovs

/lgtm

openshift-ci-robot · 2020-02-26T17:57:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: steveeJ, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [steveeJ,vrutkovs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 24, 2020

openshift-ci-robot requested review from rrati and wking February 24, 2020 21:27

steveej force-pushed the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch from d99e81a to e50d406 Compare February 24, 2020 21:27

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2020

steveej force-pushed the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch from e50d406 to 89cd657 Compare February 24, 2020 21:44

wking reviewed Feb 24, 2020

View reviewed changes

wking mentioned this pull request Feb 25, 2020

WIP: hack/graph-util: Add 'print' to write JSON to stdout openshift/cincinnati-graph-data#47

Closed

steveej mentioned this pull request Feb 25, 2020

graph-builder: use openshift/cincinnati-graph-data instead of quay labels #232

Merged

6 tasks

vrutkovs reviewed Feb 25, 2020

View reviewed changes

steveej mentioned this pull request Feb 25, 2020

graph-builder: add plugin to fetch openshift/cincinnati-graph-data from github #226

Merged

7 tasks

steveej force-pushed the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch from 89cd657 to a44fe52 Compare February 25, 2020 14:41

steveej added 2 commits February 26, 2020 10:35

dist/prow_yaml_lint: ignore the test fixtures

f8ac699

commons/testing: use log::trace instead of println

592f1af

steveej force-pushed the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch 2 times, most recently from a7768b3 to 4f3357f Compare February 26, 2020 09:42

steveej requested review from LalatenduMohanty and removed request for rrati February 26, 2020 09:44

steveej force-pushed the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch from 4f3357f to 1c35ec0 Compare February 26, 2020 14:21

steveej added 4 commits February 26, 2020 18:47

cincinnati/plugins/{node_add_remove,edge_add_remove}: make DEFAULT_KE…

ac2cef9

…Y_FILTER public

graph-builder/Cargo: stream feature for tokio; pretty_assertions

e661734

steveej force-pushed the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch from 1c35ec0 to 09fcff9 Compare February 26, 2020 17:48

vrutkovs approved these changes Feb 26, 2020

View reviewed changes

openshift-ci-robot assigned vrutkovs Feb 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020

openshift-merge-robot merged commit 7f68cf5 into openshift:master Feb 26, 2020

steveej deleted the pr/plugin-parse-openshift-cincinnati-graph-metadatag branch March 20, 2020 20:39

graph-builder: add plugin to parse openshift/cincinnati-graph-data #231

graph-builder: add plugin to parse openshift/cincinnati-graph-data #231

Uh oh!

Conversation

steveej commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LalatenduMohanty Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrutkovs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveej Feb 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveej Feb 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveej commented Feb 25, 2020

Uh oh!

steveej commented Feb 25, 2020

Uh oh!

steveej commented Feb 26, 2020

Uh oh!

steveej commented Feb 26, 2020

Uh oh!

LalatenduMohanty commented Feb 26, 2020

Uh oh!

steveej commented Feb 26, 2020

Uh oh!

vrutkovs commented Feb 26, 2020

Uh oh!

steveej commented Feb 26, 2020

Uh oh!

vrutkovs left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Feb 26, 2020

Uh oh!

steveej commented Feb 24, 2020 •

edited

Loading

LalatenduMohanty Feb 26, 2020 •

edited

Loading

steveej Feb 25, 2020 •

edited

Loading

steveej Feb 26, 2020 •

edited

Loading