-
Notifications
You must be signed in to change notification settings - Fork 7
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
refactor: use mockery-generated mock, remove gomock mock and hostcaller fake #86
refactor: use mockery-generated mock, remove gomock mock and hostcaller fake #86
Conversation
862e9c5
to
451bc05
Compare
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
451bc05
to
6bfef64
Compare
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.
LGTM, I just left one minor comment
@@ -17,4 +17,4 @@ deps: | |||
go get github.com/golangci/golangci-lint/cmd/golangci-lint | |||
|
|||
generate-mocks: | |||
docker run --rm -v ${PWD}:/src -w /src golang:1.20-alpine ./hack/generate-mocks.sh | |||
mockery |
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've seen from the mockery documentation that's it's possible to generate the mocks with go generate
. Can we do that as well, the main reason would to not require the mockery
binary to be installed on the system
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.
@flavio go generate
requires the binary to be installed, as it just runs the command in the comment. I prefer having the .mockery.yaml
configuration since it is easier to read than a golang comment.
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'm ok with the approach. Also, the Makefile is a bit of a mess.. no phony targets and so. But not needed to fix in this PR.
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.
@viccuad could you open an issue about the Makefile?
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.
LGTM
Description
Generates a new mock with mockery and removes the
gomock
(deprecated) generated mock and the hostcaller fake.Updates tests accordingly.