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

chore: Follow-up to Mockery followup #288

Merged
merged 5 commits into from Mar 25, 2024
Merged

chore: Follow-up to Mockery followup #288

merged 5 commits into from Mar 25, 2024

Conversation

lantoli
Copy link
Member

@lantoli lantoli commented Mar 25, 2024

Description

Follow-up for Mockery PR: #287

  • Uses Mockery binary (recommended) instead of go install
  • Add mocking to contributing guide

Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Required Checklist:

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code

Further comments

@lantoli lantoli marked this pull request as ready for review March 25, 2024 13:49
@lantoli lantoli requested a review from a team as a code owner March 25, 2024 13:49
tools/Makefile Outdated
rm -Rf ../mockadmin
mockery
curl -L -o mockery.tar.gz https://github.com/vektra/mockery/releases/download/v2.42.1/mockery_2.42.1_Linux_x86_64.tar.gz
Copy link
Collaborator

Choose a reason for hiding this comment

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

From this comment in the previous PR, this dependency would not be automatically updated, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure I understand your question

Copy link
Collaborator

@oarbusi oarbusi Mar 25, 2024

Choose a reason for hiding this comment

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

Sorry, what I meant is that as far as I understand, @wtrocki wanted the mockery dependency version to be elsewhere so that it can be automatically updated by dependabot. As it is now, I don't think that's the case

Copy link
Member

Choose a reason for hiding this comment

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

Related to the #288 (comment)

Re version - I think we use pattern to define them in the header of the markdown to understand how to update them and cover them in the Contributing guide

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we can use tools target as way to install all dependencies:
https://github.com/mongodb/atlas-sdk-go/blob/main/Makefile#L40

tools/Makefile Outdated
rm -Rf ../mockadmin
mockery
curl -L -o mockery.tar.gz https://github.com/vektra/mockery/releases/download/v2.42.1/mockery_2.42.1_Linux_x86_64.tar.gz
Copy link
Member

@wtrocki wtrocki Mar 25, 2024

Choose a reason for hiding this comment

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

I think we can reuse some of the scripts linked below as this solution to introduce patterns for make:

  1. Cache installation (mocking locally fails
  2. Clean up files or ignore them with git)
  3. Check if tool exist (making redundant installs without cache)

We tend to use .bin file in the repo so developers do not get tar.gz file as change when running this command.

Official docs:
https://vektra.github.io/mockery/latest/installation/

Some examples of makefile efficient installations:

  1. https://github.com/bxcodec/go-clean-arch/blob/e06c6d0cb37069b0ef56e3df67f80ca130a1ab82/misc/make/tools.Makefile#L85
  2. https://github.com/hyperledger/fabric/blob/111cff51600d26d4b4b05f52825da11e7629e971/gotools.mk#L36
  3. https://github.com/tendermint/tendermint/blob/35581cf54ec436b8c37fabb43fdaa3f48339a170/scripts/mockery_generate.sh#L9-L15

Copy link
Member Author

Choose a reason for hiding this comment

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

thank you, at the end I've followed the Docker approach so it works both in local and in the GH action

@@ -84,6 +84,7 @@ Repository contains following SDKs
- `tools`: tooling used to generate Atlas Admin SDK. See [./tools](./tools) for more information.
- `examples`: SDK examples
- `internal`: folder for hosting non user facing SDK methods and core helpers
- `mockadmin`: [Testify mocks](https://pkg.go.dev/github.com/stretchr/testify/mock) generated with [Mockery](https://github.com/vektra/mockery) for API interfaces
Copy link
Member

@wtrocki wtrocki Mar 25, 2024

Choose a reason for hiding this comment

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

IMHO we might need documentation that clarifies how we maintain mockery as part of the SDK.
Defining make target to regenerate locally and pointing to the tests example

Copy link
Member

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

LGTM although I think makefile target should support both local and github runs.

@lantoli lantoli merged commit 58e763d into main Mar 25, 2024
4 checks passed
@lantoli lantoli deleted the mockery_followup branch March 25, 2024 16:17
./mockery
.PHONY: generate_mocks
generate_mocks:
./scripts/generate_mocks.sh
Copy link
Member

Choose a reason for hiding this comment

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

❤️ Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants