-
Notifications
You must be signed in to change notification settings - Fork 47
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
Updates deppy #249
Updates deppy #249
Conversation
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I missed the change where removed returning error from NewDeppySolver
.
Else it looks good to me!
/lgtm
func (f FailEntitySource) Get(ctx context.Context, id deppy.Identifier) *input.Entity { | ||
return nil | ||
func (f FailEntitySource) Get(ctx context.Context, id deppy.Identifier) (*input.Entity, error) { | ||
return nil, fmt.Errorf("error executing get") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker at all: but do we want to define this as constants?
panicEntitySrcErr, FailEntitySrcErr etc.. just in case we want to use it in tests to assert the errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how it might be useful, but I would like to avoid doing it in this PR. Focus of this is to upgrade Deppy with minimal changes to operator controller.
This function follows the existing pattern (see Filter
, GroupBy
, etc below).
I removed |
Description
Bumps deppy to the latest commit in master.
Reviewer Checklist