operator-framework / operator-courier

Build, verify and push operators
Apache License 2.0
41 stars 53 forks source link

Provide doc links for error messages #95

Closed jeremy-wl closed 5 years ago

jeremy-wl commented 5 years ago

Problem: Currently, whenever operator-courier encounters an error, it mentions if a field is missing/invalid but does not mention how to fix the error.

Solution: Add related document links to error messages so that courier users can learn more about error and how to fix it.


Below is how I categorize the error messages, and add one or more help urls to the error message. Please comment if my categorization needs to be improved, or better links can be used for some error messages. Then I will update it in the code accordingly.

No help URLs added

_log_error("%s is not a valid email", maintainer["email"])
_log_error("%s is not a valid url", link["url"])
_log_error("Bundle does not contain any %s." % typeName)
_log_error("Bundle does not contain any %s." % typeName)
_log_error("Bundle does not contain base data field.")
_log_error("UI validation failed to verify required fields for operatorhub.io exist.")
_log_error("UI validation failed to verify that required fields for operatorhub.io are properly formatted.")
_log_error("`CRD.spec.names.plural`.`CRD.spec.group` does not match "
_log_error('CRD.spec.names.kind does not match CSV.spec.crd.owned.kind')
_log_error('CRD.spec.version does not match CSV.spec.crd.owned.version')
_log_error("channel.currentCSV %s is not included in list of csvs",
_log_error("metadata.annotations.certified is not of type string")
_log_error("no package channels defined.")
_log_error("package channel.currentCSV not defined.")
_log_error("package channel.name not defined.")
_log_error("package channels not defined.")
_log_error("packageName not defined.")
_log_error('Only 1 package is expected to exist per bundle, but got %d.', num_pkgs)
_log_error('The packageName (%s) in bundle does not match repository name (%s) provided as command line argument.'
_log_error("crd apiVersion not defined.")
_log_error("crd metadata not defined.")
_log_error("crd metadata.name not defined.")
_log_error("crd spec not defined.")
_log_error("crd spec.group not defined.")
_log_error("crd spec.names not defined.")
_log_error("crd spec.names.kind not defined.")
_log_error("crd spec.names.plural not defined.")
_log_error("crd spec.version not defined.")

invalid_category

log_error("category %s is not a valid category")

missing_owned_crd_field_in_csv

_log_error("spec.customresourcedefinitions.owned not defined for csv")
_log_error("kind not defined for item in spec.customresourcedefinitions.")
_log_error("name not defined for item in spec.customresourcedefinitions.")
_log_error("version not defined for item in spec.customresourcedefinitions.")
_log_error("custom resource definition %s referenced in csv not defined in root list of crds", csvOwnedCrd["name"])

invalid_alm_examples

_log_error("You should have alm-examples for every owned CRD")
_log_error("metadata.annotations.alm-examples contains invalid json string")

missing_field_for_operatorhub_io

_log_error("csv metadata.%s not defined. %s", field["field"], field["description"])
_log_error("csv metadata.annotations.%s not defined. %s", field["field"], field["description"])
_log_error("spec.icon can only contain two fields: \"base64data\" and \"mediatype\"")
_log_error("spec.icon should be a list")
_log_error("spec.icon should be a singleton list")
_log_error("spec.icon[0] must contain the fields \"base64data\" and \"mediatype\".")
_log_error("spec.icon[0].mediatype %s is not a valid mediatype.")
_log_error("spec.version %s is not a valid semver (example of a valid semver is: 1.0.12)",
_log_error("metadata.annotations.capabilities %s is not a valid capabilities level", annotations["capabilities"])

csv_field_issue

_log_error("csv %s not defined.", field)
_log_error("csv apiVersion not defined.")
_log_error("csv metadata not defined.")
_log_error("csv metadata not defined.")
_log_error("csv metadata.name not defined.")
_log_error("csv metadata.name not defined.")
_log_error("csv spec not defined.")
_log_error("csv spec.%s not defined. %s", field["field"], field["description"])
_log_error("csv spec.install not defined")
_log_error("csv spec.installModes not defined")
_log_error("csv.spec.links element should contain both name and url")
_log_error("csv.spec.links must be a list of name & url pairs.")
_log_error("csv.spec.maintainers element should contain both name and email")
_log_error("csv.spec.maintainers must be a list of name & email pairs.")
_log_error("csv.spec.provider element should have a single field \"name\".")
_log_error("csv.spec.provider should be a singleton list.")
_log_error("csv.spec.provider should contain a \"name\" field.")
jeremy-wl commented 5 years ago

/cc @kevinrizza @SamiSousa @galletti94

kevinrizza commented 5 years ago

@jeremylinlin Could you provide more description of the problem you are attempting to solve in the actual commit message?

kevinrizza commented 5 years ago

@jeremylinlin @galletti94 @SamiSousa Is this something we genuinely want to do? It seems like adding links to specific pages of documentation is going to be very difficult to maintain. What happens when those docs are updated (which happens relatively frequently). There is no real guarantee that these links will continue to be available. In theory we could link to specific commit versions of these docs, but then we aren't really guaranteed that those docs are accurate anymore.

IMO it seems like we need to solve the versioning/single source/all in one CLI problem before we can comfortably do something this complex. My suggestion is that we either punt this entirely, or just link very generally to the OLM or SDK repositories. And since it seems like the latter is not particularly useful, I would recommend the former.

jeremy-wl commented 5 years ago

@kevinrizza Or maybe I can add tests that check if the URLs are available or not, if not we just change it with the updated one?

kevinrizza commented 5 years ago

@kevinrizza Or maybe I can add tests that check if the URLs are available or not, if not we just change it with the updated one?

It's not really enough that they are available. It's that the concepts match what we are describing in the links. Even then, that doesn't stop these links from breaking once someone installs this on their local.

jeremy-wl commented 5 years ago

Yeah I guess there's no effective way of handling those problems. Should I close this PR? @kevinrizza

openshift-ci-robot commented 5 years ago

@jeremylinlin: PR needs rebase.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
kevinrizza commented 5 years ago

@jeremylinlin I think it would just be simpler if we updated our readme with links to OLM's docs on creating CSVs and Operator-Registry's docs on Manifests

jeremy-wl commented 5 years ago

@kevinrizza Updated the doc on this PR.

jeremy-wl commented 5 years ago

Closing the PR in favor of https://github.com/operator-framework/operator-courier/pull/115