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 the grpc api and generate html documentation #375

Merged
merged 2 commits into from
Feb 1, 2021
Merged

Document all the grpc api and generate html documentation #375

merged 2 commits into from
Feb 1, 2021

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Dec 4, 2020 •
edited
Loading

Description

Document the grpc api and generate an HTML page that can be deployed somewhere

  • bootstrap doc
  • find a way to generate an HTML version of it
  • everybody @thebsdbox , @mmlb , @nathangoulding @kdeng3849 please please please, send PR against this one with more documentation, some of the fields and methods need love.

Why is this needed

Documentation is important.

Fixes: #219 probably many more

How Has This Been Tested?

make grpc/gen_doc and looking at the generated HTML page in doc folder

@codecov
Copy link

codecov bot commented Dec 4, 2020 •
edited
Loading

Codecov Report

Merging #375 (425c6e8) into master (55ef43e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   34.34%   34.34%           
=======================================
  Files          46       46           
  Lines        2865     2865           
=======================================
  Hits          984      984           
  Misses       1794     1794           
  Partials       87       87

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ef43e...a5e4bdd. Read the comment docs.

thebsdbox
thebsdbox suggested changes Dec 9, 2020
View reviewed changes
Copy link
Contributor

@thebsdbox thebsdbox left a comment

Choose a reason for hiding this comment

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

There are a few things that I noticed, but I think this is a great start to having documentation around the API.

protos/hardware/hardware.proto Outdated
@@ -1,3 +1,7 @@
/*
* Hardware represents a single device managed by Tinkerbell.
* It can be identifed with its own UUID but more in general it can be everything that has a Mac address.
Copy link
Contributor

Choose a reason for hiding this comment

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

everything anything that has a Mac MAC address.

protos/hardware/hardware.proto Outdated
@@ -65,63 +71,162 @@ service HardwareService {
};
}

/*
* PushRequest is the body for the Push method. It contains information about
* an hardware.
Copy link
Contributor

Choose a reason for hiding this comment

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

an a piece of hardware

protos/hardware/hardware.proto Outdated
message PushRequest {
/*
* hardware describes how the hardware you want to register to Tinkerbell looks like.
Copy link
Contributor

Choose a reason for hiding this comment

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

hHardware describes how the hardware ..

Copy link
Contributor

Choose a reason for hiding this comment

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

to Tinkerbell "and what it" looks like.

protos/hardware/hardware.proto Outdated
message PushRequest {
/*
* hardware describes how the hardware you want to register to Tinkerbell looks like.
* Hostname, mac address, DHCP, network interfaces, metadata and so on.
Copy link
Contributor

Choose a reason for hiding this comment

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

macMAC

protos/hardware/hardware.proto Outdated
Netboot netboot = 2;
}
reserved 1; // obsolete dhcp
reserved 2; // obsolete netboot
/*
* Configure the network interfaces you have in your hardware and how their
Copy link
Contributor

Choose a reason for hiding this comment

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

and how defines their

protos/workflow/workflow.proto Outdated
@@ -7,33 +11,60 @@ package jackfan.us.kg.tinkerbell.tink.protos.workflow;
import "google/api/annotations.proto";
import "google/protobuf/timestamp.proto";

/*
* WorkflowSerive exposese various capabilities when it comes to starting and
Copy link
Contributor

Choose a reason for hiding this comment

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

WorkflowSerive WorkflowService exposeses exposes

protos/workflow/workflow.proto Outdated
service WorkflowService {
/*
* CreateWorkflow targets a specific hardware and it starts from a particular
* template. From now one the selected hardware is capable of picking the
Copy link
Contributor

Choose a reason for hiding this comment

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

From now one the selected hardware

kqdeng
kqdeng previously requested changes Dec 9, 2020
View reviewed changes
protos/hardware/hardware.proto Outdated
string mac = 1;
reserved 2; // obsolete ip
reserved 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we keep the // obsolete tombstones just so we have a clear record of what was deprecated / reason why the number is reserved?

protos/workflow/workflow.proto Outdated
*/
repeated string on_timeout = 6 [deprecated = true];
/*
* On failure used to be a way to execute an action if the current one timesout
Copy link
Contributor

Choose a reason for hiding this comment

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

timesout should be times out

@gianarb gianarb requested review from thebsdbox and kqdeng December 14, 2020 10:57
@gianarb gianarb added ready-to-merge Signal to Mergify to merge the PR. and removed ready-to-merge Signal to Mergify to merge the PR. labels Dec 14, 2020
@gianarb gianarb requested review from mmlb, kqdeng and thebsdbox and removed request for thebsdbox and kqdeng January 26, 2021 14:21
@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 26, 2021
thebsdbox
thebsdbox previously approved these changes Jan 29, 2021
View reviewed changes
Gianluca Arbezzano added 2 commits February 1, 2021 21:54
Started the effort to document all our grpc api
245ede8 
Signed-off-by: Gianluca Arbezzano <[email protected]>
Fix typos
a5e4bdd 
Signed-off-by: Gianluca Arbezzano <[email protected]>
@gianarb gianarb requested review from thebsdbox and kqdeng and removed request for kqdeng February 1, 2021 20:54
@gianarb gianarb removed the request for review from kqdeng February 1, 2021 21:20
@gianarb gianarb dismissed kqdeng’s stale review February 1, 2021 21:21

Not currently looking at this project anymore :(

@gianarb gianarb merged commit 873b0e2 into tinkerbell:master Feb 1, 2021
@gianarb gianarb deleted the doc/grpc-api branch February 1, 2021 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmlb mmlb mmlb approved these changes

@kqdeng kqdeng kqdeng left review comments

@thebsdbox thebsdbox Awaiting requested review from thebsdbox

Assignees

@mmlb mmlb

@nathangoulding nathangoulding

@thebsdbox thebsdbox

@kqdeng kqdeng

Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document all public interfaces
5 participants
@gianarb @mmlb @thebsdbox @kqdeng @nathangoulding