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

Reworked Go quick start #183

Merged
merged 4 commits into from
Apr 16, 2020
Merged

Reworked Go quick start #183

merged 4 commits into from
Apr 16, 2020

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Apr 10, 2020

Might help alleviate #177?

@srini100 @jtattermusch @ejona86 - who best should be assigned to review this?

cc @lucperkins @zacharysarah @thisisnotapril

content/docs/quickstart/go.md Outdated Show resolved Hide resolved
@ejona86 ejona86 requested a review from dfawley April 10, 2020 20:10
@ejona86
Copy link
Member

ejona86 commented Apr 10, 2020

@dfawley, reassign as appropriate.

Comment on lines 108 to 117
5. Setup the example as a module:

If things go smoothly, you will see the `Greeting: Hello world` in the client side output.
```sh
$ go mod init examples/helloworld
```

6. Adjust import paths to reference local packages (rather than those from the
original `google.golang.org/grpc` module):

```sh
$ perl -pi -e 's|google.golang.org/grpc/||g' greeter_{client,server}/main.go
```
Copy link
Collaborator Author

@chalin chalin Apr 11, 2020

Choose a reason for hiding this comment

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

@dfawley - it would be great if the original example sources were already setup in this way, so that we can avoid these last two steps (steps 5 and 6). Might that be possible?

Copy link
Member

Choose a reason for hiding this comment

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

I missed this comment somehow. So make the import paths relative? I think that would be OK if it's not considered an anti-pattern (we always use absolute paths).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My concern is that having the reader follow 6 steps just to copy an example's source seems like a lot of work -- especially for a quick start :). Is there any way that we can avoid some of these 6 steps?

My original suggestion what that the helloworld example be defined as a separate module already (named, say, examples/helloworld) in the grpc-go repo, but I don't know if it makes sense to do so (that is, to define a module inside of a module).

Copy link
Member

Choose a reason for hiding this comment

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

To be fair, one whole step is changing directories.

Is there a curl command we can use to fetch the examples from github? That would avoid the need to chmod them.

The "Download" link on any directory in github links you to something like this: https://download-directory.github.io/?url=https://github.com/grpc/grpc-go/tree/master/examples/helloworld

But then it asks you for a token. 😒

My original suggestion what that the helloworld example be defined as a separate module already (named, say, examples/helloworld) in the grpc-go repo, but I don't know if it makes sense to do so (that is, to define a module inside of a module).

Yes, it's totally OK to have modules inside other modules. Such a change is a breaking change, though. But this is just an example. And there's some amount of shenanigans you may need to do to avoid the conflict of older versions of grpc-go having the same packages. I think you do this by setting the sub-module to require a version of grpc-go after (or at the same commit where) the sub-module was forked. But I'm not able to find the right documentation right now (I'll keep searching).

Copy link
Member

Choose a reason for hiding this comment

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

Of course, after 5 minutes of failed searches....then my first query after sending this turned up what I was looking for:

https://github.com/golang/go/wiki/Modules#is-it-possible-to-add-a-module-to-a-multi-module-repository

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes since you're altering the path to drop the "/grpc/" part. Without step 6 the code would refer to the .pb.go files from the official grpc module.

Copy link
Member

Choose a reason for hiding this comment

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

"Yes" step 6 would be unnecessary? Or "Yes" it would still be necessary?

If the former, we can just make that change and not do anything in the grpc-go repo (I think?).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, apologies for being unclear in my answer. Step 6 would still be necessary because go mod init google.golang.org/examples/helloworld essentially changes the path to the example. The quick start also (now) changes the path to the example.

Btw, did you mean to write: go mod init google.golang.org/grpc/examples/helloworld -- which would preserve the path to the example, merely making helloworld an independent module? In that case we run into the ambiguous import problem.

Might you want us to discuss this (well, grpc/grpc-go#3540, actually) over a VC some time tomorrow?

Copy link
Member

Choose a reason for hiding this comment

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

Btw, did you mean to write: go mod init google.golang.org/grpc/examples/helloworld -- which would preserve the path to the example, merely making helloworld an independent module? In that case we run into the ambiguous import problem.

Yes, that's what I meant, sorry. OK, I see we still would also need a go.mod inside the grpc repo so that it removes those paths from its module.

At this point, given all the constraints I can see, and the options you laid out in grpc/grpc-go#3540, what I'd prefer is adding a single go.mod to grpc/examples (3), and having the quick start guide recommend copying all of grpc/examples elsewhere to work on. If that SGTY then I don't think there is any need to meet. If you would prefer another direction or just want to explore all/some of the options further, then I'm happy to meet and am available tomorrow. Feel free to email me (or use hangouts) and we can set up a time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I answered in grpc/grpc-go#3540 (comment) -- so that we could move the discussion back over there.

@chalin
Copy link
Collaborator Author

chalin commented Apr 14, 2020

@dfawley - ETA on a review?

@chalin
Copy link
Collaborator Author

chalin commented Apr 14, 2020

@dfawley - btw, we have a "gRPC Docs Working Group" GVC at 13:30 ET in case you'd like to join. Or, if you prefer to have a focused discussion concerning this PR with me, I'm available for a VC. Otherwise, comments attached to this PR would be fine. Let me know what would be simplest for you.

content/docs/quickstart/go.md Outdated Show resolved Hide resolved
content/docs/quickstart/go.md Outdated Show resolved Hide resolved
content/docs/quickstart/go.md Outdated Show resolved Hide resolved
content/docs/quickstart/go.md Outdated Show resolved Hide resolved
content/docs/quickstart/go.md Outdated Show resolved Hide resolved
@dfawley
Copy link
Member

dfawley commented Apr 14, 2020

@dfawley - btw, we have a "gRPC Docs Working Group" GVC at 13:30 ET in case you'd like to join. Or, if you prefer to have a focused discussion concerning this PR with me, I'm available for a VC. Otherwise, comments attached to this PR would be fine. Let me know what would be simplest for you.

I had already started to review this, so I finished up & sent my comments. I can join the GVC if you'd like to discuss anything significant, but I didn't have any major comments, so I think the PR should be sufficient. Thanks!

@chalin
Copy link
Collaborator Author

chalin commented Apr 14, 2020

Thanks for the review! I'll work from your comments (probably after the meeting).

@zacharysarah
Copy link

@chalin

we have a "gRPC Docs Working Group" GVC at 13:30 ET in case you'd like to join.

I’d love to join! Is there an invitation?

@chalin
Copy link
Collaborator Author

chalin commented Apr 14, 2020

@zacharysarah - I've asked @jtattermusch to add you to the weekly meeting invite.

content/docs/quickstart/go.md Outdated Show resolved Hide resolved
@chalin chalin removed the ⚠ WIP label Apr 14, 2020
@chalin chalin requested a review from dfawley April 14, 2020 21:10
@chalin
Copy link
Collaborator Author

chalin commented Apr 14, 2020

@dfawley - done with changes PTAL

@chalin
Copy link
Collaborator Author

chalin commented Apr 15, 2020

@lucperkins - did you want to review this?

@chalin chalin merged commit ea72ee4 into master Apr 16, 2020
@chalin chalin deleted the chalin-go-qs-200410 branch April 16, 2020 15:56
@chalin chalin mentioned this pull request Apr 16, 2020
@chalin
Copy link
Collaborator Author

chalin commented Apr 16, 2020

Thanks for the approval @dfawley!

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.

4 participants