Documentation: Add documentation for hive #23746

joamaki · 2023-02-14T16:04:03Z

Add the initial hive documentation to the cilium docs

joamaki · 2023-02-21T14:22:12Z

Reviewers: please consider checking this branch out (gh pr checkout 23746) and reading the docs with make -C Documentation live-preview, and also playing with the example in pkg/hive/example. I'd be really interested in hearing how these docs work for you and what's potentially missing or confusing. Thanks!

qmonnet · 2023-02-21T14:28:05Z

Reviewers: please consider checking this branch out (gh pr checkout 23746) and reading the docs with make -C Documentation live-preview

Or just use the Netlify preview? Or is there any difference that you expect?

joamaki · 2023-02-21T14:42:21Z

Reviewers: please consider checking this branch out (gh pr checkout 23746) and reading the docs with make -C Documentation live-preview

Or just use the Netlify preview? Or is there any difference that you expect?

Doh, had completely forgotten about that. No I don't expect any difference.

joamaki · 2023-02-21T14:42:43Z

https://deploy-preview-23746--docs-cilium-io.netlify.app/contributing/development/hive.html

bimmlerd

Couple of nits, but generally awesome!

Documentation/contributing/development/hive.rst

bimmlerd · 2023-02-21T14:37:13Z

Documentation/contributing/development/hive.rst

+
+    package server
+
+    // Cell implements a generic HTTP server. Provides the 'Server' API


At this point, saying just Cell implements ... it sounds like you're talking about Cells in general. Maybe

Suggested change

// Cell implements a generic HTTP server. Provides the 'Server' API

// This cell implements a generic HTTP server. Provides the 'Server' API

I briefly thought calling it ServerCell would be better, but it stutters (server.ServerCell). So maybe differentiate Identifiers from concepts using quoting, maybe `Cell` implements...?

changed it to The server cell implements... and kept it as server.Cell

bimmlerd · 2023-02-21T14:41:05Z

Documentation/contributing/development/hive.rst

+       "HTTP Server", // Module title (for documentation)
+
+       // Provide "provides" the application with a constructor for one or more
+       // objects. It is lazy and won't be called unless Invoke()'d (directly, or


On the fence about this one, but calling out the lazyness at this point already I think is a bit premature. I think the purpose of Provide is enough to take in at this stage, and the lazyness can come up in the more detailed intro to it.

In addition, I think here it could just be Provide the application with a constructor for the server, ie keep it concrete to the example.

Yeah I think it's fine to drop mentioning that it's lazy. I'm anyway mentioning it later.

Documentation/contributing/development/hive.rst

bimmlerd · 2023-02-21T14:50:07Z

Documentation/contributing/development/hive.rst

+    // This cell isn't a Module, but rather just a plain Invoke. An Invoke
+    // is a cell that, unlike Provide, is always executed. Invoke functions
+    // can depend on values that constructors registered with Provide() can
+    // return. These constructors are then called and their results remembered.


mhh maybe this makes it tricky to remove the lazyness intro above. I still think we'd gain clarity but unsure how to then explain this invoke. The Invoke, in general, I think is the most difficult to grasp intuitively based on the name. 🤔

bimmlerd · 2023-02-21T14:51:15Z

Documentation/contributing/development/hive.rst

+    func registerHelloHandler(srv server.Server) {
+        srv.RegisterHandler("/hello", helloHandler)
+    }
+


how about another quick break, maybe

Now, to put it all together, let's combine our cells into hive in the main package.

Added couple breaks.

bimmlerd · 2023-02-21T14:52:45Z

Documentation/contributing/development/hive.rst

+        // to fill it from the command-line flags.
+        flags.Int("server-port",  def.ServerPort, "Sets the HTTP server listen port")
+    }
+


mabe a quick sentence that this is now a separate package and file, something like

Let's now look at a consumer of this cell, the hello package.

Documentation/contributing/development/hive.rst

bimmlerd · 2023-02-21T15:03:47Z

Documentation/contributing/development/hive.rst

+
+    cilium$ go run ./daemon hive dot-graph | dot -Tx11
+
+Guidelines


These deserve an introduction sentence. Maybe even call out specifically that these can and should be used as Code review comments like https://github.com/golang/go/wiki/CodeReviewComments

Documentation/contributing/development/hive.rst

Documentation/spelling_wordlist.txt

qmonnet · 2023-02-21T16:06:21Z

Documentation/spelling_wordlist.txt

@@ -680,6 +692,7 @@ mediabot
 memcache
 memcached
 memcd
+memoized


Typo on this one, please fix the docs and remove it from this file.

Don't see the typo, but rephrased it anyway.

OK my bad, I didn't know the term (and thought you meant memorized, obviously)

Then it's best I changed it to something less obscure :D

Documentation/contributing/development/hive.rst

qmonnet · 2023-02-21T16:09:40Z

Documentation/contributing/development/hive.rst

+
+    cilium$ go run ./daemon hive dot-graph | dot -Tx11
+
+Guidelines


Do we have any kind of CI check to enforce these guidelines? To me, some of them look easy to miss, and developers won't always think of checking this list.

There's a test (daemon/cmd/cells_test.go) checks that the hive can be instantiated which potentially catches side-effects in the constructors. For the rest they need to be specifically tested for (e.g. cell's integration test checks with goleak that Stop() cleans up). If I do come up with guidelines and a generic way of testing for it I'll definitely implement it. (Already have some ideas about being able to enforce dependency direction...)

Documentation/contributing/development/hive.rst

pkg/hive/example/mini/main.go

Documentation/contributing/development/hive.rst

pippolo84

I still have to review all the example code yet, but docs looks good to me, minus other reviewers comments.
Just a nit left inline.

Documentation/contributing/development/hive.rst

qmonnet

Found one typo (this time I think it's a real one :) ).

The rest looks all good from my side, thanks for addressing the feedback. The intro reads much better!

Documentation/contributing/development/hive.rst

The prior example was a bit too abstract, so rewrite it instead to showcase hive with an HTTP server with pluggable handlers. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Add a very minimal example of how something like uber/dig is implemented on top of the "reflect" package for those curious about the internals. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Add the initial hive documentation to the cilium docs. Signed-off-by: Jussi Maki <jussi@isovalent.com>

qmonnet

Thanks!

sayboras · 2023-02-24T23:18:12Z

Full CI is not required, and seems like most (if not all) of the comments are addressed. Merging.

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 14, 2023

joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Feb 14, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 14, 2023

joamaki force-pushed the pr/joamaki/hive-docs branch from c245174 to bd1625f Compare February 21, 2023 14:19

joamaki marked this pull request as ready for review February 21, 2023 14:20

joamaki requested review from a team as code owners February 21, 2023 14:20

joamaki requested review from pippolo84, qmonnet and bimmlerd February 21, 2023 14:20

bimmlerd approved these changes Feb 21, 2023

View reviewed changes

qmonnet reviewed Feb 21, 2023

View reviewed changes

qmonnet added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Feb 21, 2023

tklauser reviewed Feb 21, 2023

View reviewed changes

pippolo84 reviewed Feb 22, 2023

View reviewed changes

Documentation/contributing/development/hive.rst Outdated Show resolved Hide resolved

joamaki force-pushed the pr/joamaki/hive-docs branch 2 times, most recently from 539f3c4 to 2814a79 Compare February 22, 2023 17:46

joamaki requested a review from qmonnet February 22, 2023 17:47

joamaki force-pushed the pr/joamaki/hive-docs branch from 2814a79 to b4e6d43 Compare February 23, 2023 10:21

qmonnet reviewed Feb 24, 2023

View reviewed changes

Documentation/contributing/development/hive.rst Outdated Show resolved Hide resolved

joamaki added 3 commits February 24, 2023 13:20

hive/example: Rewrite the example as a HTTP server

59ea33c

The prior example was a bit too abstract, so rewrite it instead to showcase hive with an HTTP server with pluggable handlers. Signed-off-by: Jussi Maki <jussi@isovalent.com>

hive/example: Add "mini", a minimal example of reflect based DI

9caf3e6

Add a very minimal example of how something like uber/dig is implemented on top of the "reflect" package for those curious about the internals. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Documentation: Add documentation for hive

12c6d51

Add the initial hive documentation to the cilium docs. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/joamaki/hive-docs branch from b4e6d43 to 12c6d51 Compare February 24, 2023 12:22

qmonnet approved these changes Feb 24, 2023

View reviewed changes

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 24, 2023

sayboras merged commit 365e19e into cilium:master Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Documentation: Add documentation for hive #23746

Documentation: Add documentation for hive #23746

joamaki commented Feb 14, 2023

joamaki commented Feb 21, 2023

qmonnet commented Feb 21, 2023

joamaki commented Feb 21, 2023

joamaki commented Feb 21, 2023

bimmlerd left a comment

bimmlerd Feb 21, 2023

joamaki Feb 22, 2023

bimmlerd Feb 21, 2023

joamaki Feb 22, 2023

bimmlerd Feb 21, 2023

bimmlerd Feb 21, 2023

joamaki Feb 22, 2023

bimmlerd Feb 21, 2023

joamaki Feb 22, 2023

bimmlerd Feb 21, 2023

joamaki Feb 22, 2023

qmonnet Feb 21, 2023

joamaki Feb 22, 2023

qmonnet Feb 24, 2023 •

edited

joamaki Feb 24, 2023

qmonnet Feb 21, 2023

joamaki Feb 22, 2023

pippolo84 left a comment

qmonnet left a comment

qmonnet left a comment

sayboras commented Feb 24, 2023


		package server

		// Cell implements a generic HTTP server. Provides the 'Server' API

	// Cell implements a generic HTTP server. Provides the 'Server' API
	// This cell implements a generic HTTP server. Provides the 'Server' API


		cilium$ go run ./daemon hive dot-graph \| dot -Tx11

		Guidelines

Documentation: Add documentation for hive #23746

Documentation: Add documentation for hive #23746

Conversation

joamaki commented Feb 14, 2023

joamaki commented Feb 21, 2023

qmonnet commented Feb 21, 2023

joamaki commented Feb 21, 2023

joamaki commented Feb 21, 2023

bimmlerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qmonnet Feb 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pippolo84 left a comment

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

qmonnet left a comment

Choose a reason for hiding this comment

sayboras commented Feb 24, 2023

qmonnet Feb 24, 2023 •

edited