Check presence and match of fields in CRD and CSV in bundle #41

jeremy-wl · 2019-03-07T14:49:03Z

Check if the following fields in CRD matches that in CSV in bundle
- CRD.spec.names.plural.CRD.spec.group == CSV.spec.crd.owned.name
- CRD.spec.names.kind == CSV.spec.crd.owned.kind
- CRD.spec.version == CSV.spec.crd.owned.version
Check the presence of all above fields in the bundle

jeremy-wl · 2019-03-07T16:51:07Z

/cc @kevinrizza @awgreene

awgreene

Nice work @jeremylinlin

operatorcourier/validate.py

tests/test_files/bundles/verification/crdmissingkindfield.invalid.bundle.yaml

operatorcourier/validate.py

jeremy-wl · 2019-03-11T20:05:17Z

/cc @awgreene @kevinrizza

kevinrizza

Nice work on this. I just had one small comment, otherwise looks great.

operatorcourier/validate.py

awgreene

Please address the comments.

jeremy-wl · 2019-03-12T16:11:32Z

/cc @kevinrizza @awgreene

kevinrizza · 2019-03-14T18:13:41Z

/lgtm

SamiSousa

Great work on this! Mostly nit-picks on style, I think some logs need additional info as well

SamiSousa · 2019-03-15T18:14:53Z

operatorcourier/validate.py

-                self._log_error("crd apiVersion not defined.")
-                valid = False
+            for crd_required_nested_field in self.crd_required_nested_fields:
+                valid &= self._crd_fields_validation(crd_required_nested_field.split('.'),


nit: Any benefit of using this oneliner over an if statement? It's already long enough to spread two lines

Actually using an if statement will also make the line exceed 90 chars. I think the line is long mainly because the long and descriptive method and variable names.

Please be aware, that &= is a bitwise operator. But it seems this code intends to do a boolean operation, so probably

valid = valid and crd_field_valid

would better express the intention here.

Hi @csomh, thanks for the comment. I have updated this.

SamiSousa · 2019-03-15T18:18:10Z

operatorcourier/validate.py

+        if root_field == 'metadata' and 'name' in yaml_dict['metadata']:
+            logging.info("Evaluating crd %s", yaml_dict['metadata']['name'])
+        if root_field not in yaml_dict:
+            self._log_error("crd %s not defined.", field_string)


nit: Can this also print the name of the crd in question? Or is that covered by previous if statement?

Or is that covered by previous if statement?

Yes.

Can this also print the name of the crd in question?

This actually prints out the (nested) field that is missing in the CRD. See this test case as an example. I have updated the log message to make it less confusing.

operatorcourier/validate.py

SamiSousa · 2019-03-15T18:21:12Z

operatorcourier/validate.py

+                                    "spec.customresourcedefinitions.")
+                    valid = False
+                elif csvOwnedCrd["name"] not in crdList:
+                    self._log_error("custom resource definition referenced in csv "


Can this print the name of the CRD that doesn't match?

Can the CRD name also be added to the other error logs here as well?

Actually I didn't change much logic in those if statements except making the code less indented.

Can this print the name of the CRD that doesn't match?

Sure I can do that.

Can the CRD name also be added to the other error logs here as well?

I can but that will require some additional nested if checks to make sure csvOwnedCrd["name"] exists and is valid. Do you want me to do that?

SamiSousa · 2019-03-15T18:23:44Z

operatorcourier/validate.py

+                    valid = False
+
+                for crd in bundleData[self.crdKey]:
+                    if 'name' not in csvOwnedCrd or not \


nit: This if statement is kinda long, and I'm not sure what it's checking for: Are we continuing if the name doesn't exist or if it does exist?

We continue to check field matches only if the name in both CRD and CSV exists, and they match.

SamiSousa · 2019-03-20T18:38:35Z

operatorcourier/validate.py

+
+                for crd in bundleData[self.crdKey]:
+                    if 'name' not in csvOwnedCrd or not \
+                        self._crd_fields_validation(['metadata', 'name'], crd, '') \


If a CRD in the bundle doesn't have a name, should that also log as an error?

Hi @SamiSousa, thanks for the review. Yes, but I believe this should be covered in previous checks.

That check is for the CRD in the CSV, rather than the CRD from the bundle directly.

operator-courier/operatorcourier/validate.py

Line 166 in 25af109

crdList = []

On this line the crdList is created. Should this be used in this for loop?

Yeah, for each type we should have a list of fields we check. The fact that they happen to be checked in other places by code that compares those fields to other fields shouldn't be relevant. I think part of the problem is that adding this required fields list crd_required_nested_fields makes the code hard to read.

Perhaps instead we should either do that everywhere for every type, or not create that abstraction at all. Either way, it actually seems outside the scope of this pr. Can we revert it?

@kevinrizza Without this abstraction we will need to introduce even more nested ifs just to check the presence of nested fields. These are already checked in CRD checks but we need to do it again to make sure the field in the yaml we compare against csvOwnedCrd actually exists.

Maybe after this PR I can apply this abstraction for checks in all types? And this hopefully can reduce the nested levels like this that can be found in multiple places in this file.

Your intuition about nested ifs is good in the general case, but I think in this case it makes the code more clear. If we abstract away certain steps but only in certain cases, it makes the code hard to reason about.

Additionally, I think ultimately implementing a change like the one described here is probably the better long term approach: #39

@kevinrizza That seems really cool and should be the better way to do it. For this particular PR, what are the changes you want me to make? Should I remove this abstraction and replace it with nested ifs instead?

@jeremylinlin yes, and please ensure that any of the checks were doing before are no longer removed.

/cc @kevinrizza @SamiSousa Updated.

- Check if the following fields in CRD matches that in CSV in bundle - `CRD.spec.names.plural`.`CRD.spec.group` == `CSV.spec.crd.owned.name` - `CRD.spec.names.kind` == `CSV.spec.crd.owned.kind` - `CRD.spec.version` == `CSV.spec.crd.owned.version` - Check the presence of all above fields in the bundle

SamiSousa

/lgtm

SamiSousa · 2019-03-27T18:23:44Z

operatorcourier/validate.py

+                    if csvOwnedCrd['name'] != crd['metadata']['name']:
+                        continue
+
+                    if 'kind' in csvOwnedCrd:


This is much easier to read. 🤓

kevinrizza · 2019-03-29T12:16:12Z

/lgtm

openshift-ci-robot requested review from awgreene and kevinrizza March 7, 2019 14:49

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 7, 2019

awgreene reviewed Mar 7, 2019

View reviewed changes

operatorcourier/validate.py Show resolved Hide resolved

tests/test_files/bundles/verification/crdmissingkindfield.invalid.bundle.yaml Show resolved Hide resolved

kevinrizza requested changes Mar 7, 2019

View reviewed changes

operatorcourier/validate.py Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 8, 2019

jeremy-wl force-pushed the check-csv-crd-fields-presence-and-match branch 2 times, most recently from 500032d to 6aef00d Compare March 11, 2019 20:02

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2019

openshift-ci-robot requested review from awgreene and kevinrizza March 11, 2019 20:05

kevinrizza requested changes Mar 12, 2019

View reviewed changes

operatorcourier/validate.py Outdated Show resolved Hide resolved

awgreene requested changes Mar 12, 2019

View reviewed changes

jeremy-wl closed this Mar 12, 2019

jeremy-wl force-pushed the check-csv-crd-fields-presence-and-match branch from 6aef00d to 4628bfa Compare March 12, 2019 15:48

openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 12, 2019

jeremy-wl reopened this Mar 12, 2019

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 12, 2019

openshift-ci-robot requested review from awgreene and kevinrizza March 12, 2019 16:11

openshift-ci-robot assigned kevinrizza Mar 14, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 14, 2019

SamiSousa suggested changes Mar 15, 2019

View reviewed changes

jeremy-wl force-pushed the check-csv-crd-fields-presence-and-match branch from e4c3841 to 3f168f9 Compare March 15, 2019 20:05

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 15, 2019

jeremy-wl force-pushed the check-csv-crd-fields-presence-and-match branch from 3f168f9 to 25af109 Compare March 18, 2019 22:38

SamiSousa suggested changes Mar 20, 2019

View reviewed changes

jeremy-wl force-pushed the check-csv-crd-fields-presence-and-match branch from 25af109 to 08caa95 Compare March 20, 2019 22:19

jeremy-wl force-pushed the check-csv-crd-fields-presence-and-match branch from 08caa95 to 85d79bc Compare March 20, 2019 22:23

SamiSousa approved these changes Mar 27, 2019

View reviewed changes

openshift-ci-robot assigned SamiSousa Mar 27, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2019

kevinrizza merged commit 9f7d4ce into operator-framework:master Mar 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check presence and match of fields in CRD and CSV in bundle #41

Check presence and match of fields in CRD and CSV in bundle #41

jeremy-wl commented Mar 7, 2019

jeremy-wl commented Mar 7, 2019

awgreene left a comment

jeremy-wl commented Mar 11, 2019

kevinrizza left a comment

awgreene left a comment

jeremy-wl commented Mar 12, 2019

kevinrizza commented Mar 14, 2019

SamiSousa left a comment

SamiSousa Mar 15, 2019

jeremy-wl Mar 15, 2019 •

edited

Loading

csomh Mar 18, 2019

jeremy-wl Mar 18, 2019

SamiSousa Mar 15, 2019

jeremy-wl Mar 15, 2019

SamiSousa Mar 15, 2019

SamiSousa Mar 15, 2019

jeremy-wl Mar 15, 2019

SamiSousa Mar 15, 2019

jeremy-wl Mar 15, 2019

SamiSousa Mar 20, 2019

jeremy-wl Mar 20, 2019

SamiSousa Mar 20, 2019

kevinrizza Mar 20, 2019

jeremy-wl Mar 20, 2019

kevinrizza Mar 20, 2019

jeremy-wl Mar 20, 2019 •

edited

Loading

kevinrizza Mar 20, 2019

jeremy-wl Mar 20, 2019

SamiSousa left a comment

SamiSousa Mar 27, 2019

kevinrizza commented Mar 29, 2019

Check presence and match of fields in CRD and CSV in bundle #41

Check presence and match of fields in CRD and CSV in bundle #41

Conversation

jeremy-wl commented Mar 7, 2019

jeremy-wl commented Mar 7, 2019

awgreene left a comment

Choose a reason for hiding this comment

jeremy-wl commented Mar 11, 2019

kevinrizza left a comment

Choose a reason for hiding this comment

awgreene left a comment

Choose a reason for hiding this comment

jeremy-wl commented Mar 12, 2019

kevinrizza commented Mar 14, 2019

SamiSousa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy-wl Mar 15, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy-wl Mar 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamiSousa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrizza commented Mar 29, 2019

jeremy-wl Mar 15, 2019 •

edited

Loading

jeremy-wl Mar 20, 2019 •

edited

Loading