Skip to content

Conversation

jchunkins
Copy link
Contributor

@jchunkins jchunkins commented Feb 25, 2021

This is the first step in implementing issue operator-framework/operator-registry#568

I started with the annotations that I knew were problematic (e.g. using wrong case olm.skiprange when the annotation should be olm.skipRange) and also searched for other case sensitive annotations I could find. I also expanded the validation types to include OperatorGroups as well since it can also use case sensitive annotation names. Note that OperatorGroups are not directly hooked up to the command line execution of operator-verify (similar to how PackageManifestValidator is not accessible from the command line either).

I intend to open up new PRs in operator-registry once this is merged here so we can pick up the changes for CSV validation.

  • Add validation function for well known case sensitive annotation names
  • Use new validation function in CSV and OperatorGroups
  • Add minimal OperatorGroup validator calling new function
  • Add / update unit tests and test data
  • update usage readme

Signed-off-by: John Hunkins jhunkins@us.ibm.com

Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

Lookin good. Just a few comments.

@jchunkins jchunkins requested a review from dinhxuanvu March 3, 2021 21:13
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2021
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Great work @jchunkins, left some non-blocking nits.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2021
- Add validation function for well known case sensitive annotation names
- Use new validation function in CSV and OperatorGroups
- Add minimal OperatorGroup validator calling new function
- Add / update unit tests and test data
- update usage readme

Signed-off-by: John Hunkins <jhunkins@us.ibm.com>
@jchunkins jchunkins force-pushed the operator_registgy_issue_568 branch from b1cac36 to 1ce77c8 Compare March 5, 2021 16:25
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2021
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

Ah, this looks interesting! Although it does seem fairly limited; i.e. in order to catch similar mistakes (e.g. olm.OperatorGroups, olm.operator-group, etc), we need to add more custom logic.

Have you considered something more generalized? maybe this could use a string distance algorithm (see agnivade/levenschtein) to catch non-exact matches and suggest the closest correct key?

@jchunkins
Copy link
Contributor Author

@njhale I appreciate the idea and can see how that can make this more useful. However, considering a crawl/walk/run approach, maybe we start off with this implementation before committing to additional complexity.

My primary concern would be introducing false positives when you start handling more complex scenarios. A secondary concern would be the computation time cost involved during validation (which admittedly might not be that large). Right now we know that we have real world examples of folks making simple mistakes with case sensitivity being the root problem.

Copy link
Contributor

@benluddy benluddy left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I don't have a problem with a specific case-sensitivity rule like this one.

It might eventually be useful to validate that any annotation key beginning with "olm." (?) can be found on an annotation allowlist, which would cover the other mistakes Nick brought up. That would take a bit of extra effort to offer a "did you mean" like the one your patch provides, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants