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

Scaffold identity server #371

Merged
merged 8 commits into from
Apr 11, 2024
Merged

Scaffold identity server #371

merged 8 commits into from
Apr 11, 2024

Conversation

richardhuaaa
Copy link
Contributor

@richardhuaaa richardhuaaa commented Apr 11, 2024

This generates the protos, and stubs out the identity service API endpoints. I'll work on the DB changes next.

The existing unit tests pass, but to verify this actually did something, is the idea to land this, and then run curl against the endpoints on the GRPC gateway on dev?

@@ -8,7 +8,7 @@ if ! which golangci-lint &>/dev/null; then brew install golangci-lint; fi
if ! which shellcheck &>/dev/null; then brew install shellcheck; fi
if ! which protoc &>/dev/null; then brew install protobuf; fi
if ! which protoc-gen-go &>/dev/null; then go install google.golang.org/protobuf/cmd/protoc-gen-go@latest; fi
if ! which mockgen &>/dev/null; then go install go.uber.org/mock/mockgen@latest; fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was a pain to find, but tests were failing until I did this.

Basically, it's entirely possible you have a different version of mockgen installed on your system (in fact, you're guaranteed to have it if you have developed against xmtp-node-go in the past). That would be github.com/golang/mock/mockgen instead of this uber one.

The reason for the use of the uber one seems to be the .Satisfied property, which lets you check on an interval if all of the expected API calls were performed on the mock. We're using it in tests like this:

require.Eventually(t, ctrl.Satisfied, 5*time.Second, 100*time.Millisecond)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch

@richardhuaaa richardhuaaa requested review from neekolas and a team April 11, 2024 01:09
@richardhuaaa richardhuaaa marked this pull request as ready for review April 11, 2024 01:10
dev/up Outdated
@@ -8,7 +8,7 @@ if ! which golangci-lint &>/dev/null; then brew install golangci-lint; fi
if ! which shellcheck &>/dev/null; then brew install shellcheck; fi
if ! which protoc &>/dev/null; then brew install protobuf; fi
if ! which protoc-gen-go &>/dev/null; then go install google.golang.org/protobuf/cmd/protoc-gen-go@latest; fi
if ! which mockgen &>/dev/null; then go install go.uber.org/mock/mockgen@latest; fi
go install go.uber.org/mock/mockgen@latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there no way to tell if you have the correct mockgen and only run this if you have the wrong one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I found a hack to only run this if the version is not 0.4.0. Basically we can pin the uber mockgen on the current version of v0.4.0 and update as needed. Should be good indefinitely because it is running behind the golang one, which is on v1.6.0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice work!

@@ -15,6 +15,8 @@ After `xmtpd` meets specific functional requirements, the plan is for it to beco
- [Go](https://go.dev/doc/install)
- [Docker](https://www.docker.com/get-started/)

You must have the _exact_ go version listed in `go.mod` - you can verify this by running `go version`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Of all our projects, this is probably the one that needs a devcontainer the most come to think of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, we should probably invest if more people start working on the backend

@richardhuaaa richardhuaaa merged commit 4c761e5 into main Apr 11, 2024
3 checks passed
@richardhuaaa richardhuaaa deleted the rich/scaffold-identity branch April 11, 2024 18:39
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.

2 participants