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

Depreciate ErrNoOperatorCondition #65

Merged

Conversation

awgreene
Copy link
Member

@awgreene awgreene commented Jul 6, 2021

Problem: The ErrNoOperatorCondition does not surface any errors
encountered when retrieving OperatorConditions from the cluster.
It is not possible to know if there is an RBAC issue, if the
resource does not exist, or some other error.

Solution: Surface the error retrieving the OperatorCondition

@coveralls
Copy link

coveralls commented Jul 6, 2021

Pull Request Test Coverage Report for Build 1008560611

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.187%

Totals Coverage Status
Change from base Build 1005607081: 0.0%
Covered Lines: 331
Relevant Lines: 418

💛 - Coveralls

Copy link
Member

@varshaprasad96 varshaprasad96 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 openshift-ci bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 6, 2021
Problem: The ErrNoOperatorCondition does not surface any errors
encountered when retrieving OperatorConditions from the cluster.
It is not possible to know if there is an RBAC issue, if the
resource does not exist, or some other error.

Solution: Surface the error retrieving the OperatorCondition.
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 7, 2021
@varshaprasad96
Copy link
Member

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: varshaprasad96

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2021
@openshift-merge-robot openshift-merge-robot merged commit 59dbe82 into operator-framework:main Jul 7, 2021
@joelanford
Copy link
Member

Just a belated nit: Since we no longer return this error, I think we should do one more thing.
Either:

  • Add a bit more the the Deprecated comment (and fix the misspelling so it shows up in IDEs) to explain that this error is no longer returned and users should always expect the more generic error from the client
  • Just go ahead and remove it (we're not v1 in operator-lib yet, so breaking changes are alright).

@awgreene
Copy link
Member Author

awgreene commented Jul 7, 2021

Just go ahead and remove it (we're not v1 in operator-lib yet, so breaking changes are alright).

+1

@awgreene
Copy link
Member Author

awgreene commented Jul 7, 2021

@joelanford I created #66 to remove the ErrNoOperatorCondition.

@estroz
Copy link
Member

estroz commented Jul 7, 2021

A 3rd option: wrap the underlying error in ErrNoOperatorCondition, which is my preference.

@awgreene
Copy link
Member Author

awgreene commented Jul 7, 2021

@estroz what value did we get from the ErrNoOperatorCondition error?

@estroz
Copy link
Member

estroz commented Jul 7, 2021

I don't know, I was assuming there was a particular reason it was added in the first place. If you don't think there is benefit, then I'm in favor of removing the error entirely.

@joelanford
Copy link
Member

It seems like the intention was to signal not found, but I agree with @awgreene's premise that:
a) it was swallowing and translating non-non found errors, which could give callers an incorrect impression
b) callers can just use apierrors.IsNotFound() to test the equivalent of what ErrNoOperatorCondition seems like it was meant to convey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants