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.

Sign up for GitHub

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

Jump to bottom

Document all public interfaces #219

Closed
displague opened this issue Jul 16, 2020 · 1 comment · Fixed by #375
Closed

Document all public interfaces #219

displague opened this issue Jul 16, 2020 · 1 comment · Fixed by #375
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. hacktoberfest size/M estimate of the amount of work to address the issue

Comments

@displague
Copy link
Member

displague commented Jul 16, 2020 •
edited
Loading

Expected Behaviour

As a developer, the interfaces of the client should be easy to work with.

The endpoints should be documented in a way that describes what the purpose of each endpoint is and what the parameters do.

Current Behaviour

Endpoints are not documented:

tink/protos/hardware/hardware.pb.go

Line 2023 in 0100e53

func NewHardwareServiceClient(cc grpc.ClientConnInterface) HardwareServiceClient {

This makes features like #195 (OpenAPI generated documentation and clients) less valuable.

Possible Solution

Document (as code comments) each of the public functions, types, and fields for the 3 Client interfaces.
https://github.com/tinkerbell/tink/blob/master/client/main.go#L66-L68
@gianarb
Copy link
Contributor

gianarb commented Aug 5, 2020

Hello @displague ! Thank you for opening this PR. I think comments has to be improved and you identify a very valuable place to start with!

Are you interested in opening a PR with the 3 comments you suggested?!

Thanks

@gianarb gianarb added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Aug 7, 2020
@displague displague mentioned this issue Aug 7, 2020
Merged
3 tasks
mergify bot added a commit that referenced this issue Sep 18, 2020
@mergify
Add documentation to proto files (#243)
93b9338 
## Description

This PR:
* makes `protos/protoc.sh` executable in environments without protoc (and c++) installed
* docker provides a reproducible protoc environment for regenerating pb.go files (jaegertracing/protobuf, libprotoc 3.8.0)
* adds documentation to the hardware.proto file

## Why is this needed

This starts the effort needed for #219

Additional .proto file docs would be needed to close that issue.

## How Has This Been Tested?

Let me know how I should test this.

I would like to include a `make protos` task.
Should this be run by CI to check for uncommitted drift?

<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->


## How are existing users impacted? What migration steps/scripts do we need?

<!--- Fixes a bug, unblocks installation, removes a component of the stack etc -->
<!--- Requires a DB migration script, etc. -->


## Checklist:

I have:

- [x] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade
@gauravgahlot gauravgahlot added hacktoberfest size/M estimate of the amount of work to address the issue labels Sep 23, 2020
@gianarb gianarb mentioned this issue Dec 4, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. hacktoberfest size/M estimate of the amount of work to address the issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Document all the grpc api and generate html documentation
3 participants
@displague @gianarb @gauravgahlot