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

Revived the example with a new structure and docs #5

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

DennisSSDev
Copy link

Description of changes:

The docs have been updated to follow the code structure more in sync as well as utilize a more modern code generation using Makefile

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@DennisSSDev DennisSSDev added the ready for review Marks the PR ready to be viewed by the code owners label Mar 13, 2021
@mellis
Copy link

mellis commented Mar 16, 2021

  1. Why use vendoring still instead of regular modules without vendoring?
  2. Should the try it out instructions have non-GOPATH instructions since that is on the deprecation/removal path now?


docker-container: docker-image
$(info $(M) Running docker application container...)
docker run -p 8080:8080 example:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Using $(info ...) is neat, but I would remove any "advanced" features from this Makefile. Let's try to keep just the minimal to run the twirp example. Instead of that, we could add a comment.

@@ -1,4 +1,4 @@
// Copyright 2018 Twitch Interactive, Inc. All Rights Reserved.
// Copyright 2020 Twitch Interactive, Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2021 already 🎉

@marioizquierdo
Copy link
Contributor

This is great @DennisSSDev, let's try to move it to the finish line!

@DennisSSDev
Copy link
Author

DennisSSDev commented Apr 16, 2021

The vendor folder is explicitly required to generate the binary twirp tools (look at the makefile please). This is the simplest way I could make it, besides effectively forcing the end-user to manually install twirp on their machine. If we want to go towards that route, we can remove the vendor folder and make the makefile command look for a local version of the twirp tool binaries, but I think that would make playing around with this example faaar more difficult

@marioizquierdo
Copy link
Contributor

The vendor folder is explicitly required to generate the binary twirp tools

The requirement to allow protoc work with a specific version of protoc-gen-go and protoc-gen-twirp, is to have them installed in the PATH. You can do this using GOBIN when installing the tool (e.g. GOBIN=./_bin go install ...), and/or making sure that the PATH has that folder when calling protoc (e.g. PATH=./_bin:$PATH protoc ...).

The go install command will download the dependency with the right version specified in go.mod. If the vendor folder is present, then the download is not needed. But in this case, it is reasonable to require connection to Github, because this example is already downloaded from Github.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Marks the PR ready to be viewed by the code owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants