Skip to content
This repository was archived by the owner on Jan 28, 2025. It is now read-only.

Conversation

@varshaprasad96
Copy link
Member

@varshaprasad96 varshaprasad96 commented Apr 12, 2023

The "Get" function of the entity source did not return an error and instead only a nil entity when its not found. This PR modifies the same, to make it return an error.

Reason:
The method could be implmented with a client that fetches bundle contents from the cluster. The error returned from the client needs to be propagated. Even if its from an offline key-value store, it could be upto the library users to ignore the returned error.

More info: operator-framework/operator-controller#141 (comment)

The "Get" function of the entity source did not return an error and instead
only a nil entity when its not found. This PR modifies the same, to make it
return an error.

Reason:
The method could be implmented with a client that fetches bundle contents from
the cluster. The error returned from the client needs to be propagated.

Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2023
@varshaprasad96 varshaprasad96 marked this pull request as ready for review April 12, 2023 16:36
@varshaprasad96 varshaprasad96 requested a review from a team as a code owner April 12, 2023 16:36
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 12, 2023
@joelanford
Copy link
Member

Looks good to me, but I'll defer to @perdasilva to make sure we aren't missing some important context about this.

Copy link
Contributor

@ankitathomas ankitathomas 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 the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2023
Copy link

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

Lgtm

@joelanford
Copy link
Member

(Something seems off with the merge requirement for someone from operator-framework/deppy-maintainers. Me, @perdasilva, @ankitathomas , and @everettraven have all approved, and apparently none of us are on that team. I'm going to use my admin powers to merge anyway.

@joelanford joelanford merged commit 8610b34 into operator-framework:main Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants