📖 Add project overview to docs, clean up docs, structure #295

DirectXMan12 · 2019-01-16T06:40:59Z

This adds an overview of the project organization to the root godocs, and cleans up the docs here and there, fixing whitespace issues, cleaning up wording, clarifying, and adding docs where they're lacking.

This also moves things out of pkg/runtime, so that pkg/runtime is (mostly) deprecated paths to other things.

This lays out package docs for the client package, clarifies that client.New creates a new direct-to-API client, and adds an example for indexers.

This adds an overview of the library to the root package.

k8s-ci-robot · 2019-01-16T06:41:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

droot

Looks pretty good, some minor nits.

droot · 2019-01-17T17:49:42Z

doc.go

+//
+// Controllers
+//
+// Controllers (pkg/controller) handle using events (pkg/events) to eventually


s/handle using/handle

droot · 2019-01-17T17:51:53Z

doc.go

+// Logging and Metrics
+//
+// Logging (pkg/log) in controller-runtime is done via structured logs, using a
+// log interface set called logr (https://godoc.org/github.com/go-logr/logr).


s/interface set/interface ?

droot · 2019-01-17T17:53:52Z

doc.go

+//
+// Metrics (pkg/metrics) provided by controller-runtime are registered into a
+// controller-runtime-specific Prometheus metrics registery.  The manager will
+// serve these by default at an HTTP endpoint, and additional metrics may be


I don't think we serve the metrics by default. We wanted to, but couldn't so because scaffolded tests won't be able to run in parallel because of port-conflict.

droot · 2019-01-17T17:54:30Z

doc.go

+// stand up a copy of etcd and kube-apiserver, and provide the correct options
+// to connect to the API server.  It's designed to work well with the Ginkgo
+// testing framework, but should work with any testing setup.
+package controllerruntime


Nicely written overview.

droot · 2019-01-17T17:57:19Z

pkg/client/example_test.go

@@ -196,3 +198,24 @@ func ExampleClient_delete() {
 	})
 	_ = c.Delete(context.Background(), u)
 }
+
+// This example shows how to set up and consume a field select over a pod's volumes' secretName field.


s/field select/field selector

droot · 2019-01-17T18:02:34Z

pkg/manager/manager.go

@@ -84,7 +84,8 @@ type Manager interface {
 // Options are the arguments for creating a new Manager
 type Options struct {
 	// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources
-	// Defaults to the kubernetes/client-go scheme.Scheme
+	// Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always a good
+	// idea to pass your own scheme in.


I have seen users struggling with this. I think we can provide some guidance here: For ex. user is writing controllers with built-in types, then default scheme is enough, but if using your own types/external-types, augment the default scheme with the scheme for specific types.

I've added more documentation on schemes to pkg/runtime/scheme, which probably should be promoted to pkg/scheme.

This cleans up the existing docs, clarifying working, improving spacing, and making sure all packages have docs.

Manager previously conformed to a slightly different interface than recorder.Provider. This fixes that.

pkg/runtime is a bit of a odd collection right now. This starts to move stuff out of it, leaving aliases in place instead. Since signals are used to control managers, this functionality now lives in a subpackage of manager.

Continue moving stuff out of pkg/runtime. the schemebuilder now lives in pkg/scheme.

rajathagasthya · 2019-02-05T00:17:31Z

FAQ.md

+
+- When you can, take advantage of optimistic locking: use deterministic
+  names for objects you create, so that the Kubernetes API server will
+  warn you if the object already exists.  May controllers in Kubernetes


s/May/Many/

thbkrkr · 2019-02-05T10:38:18Z

FAQ.md

+
+**A**: The fake client
+[exists](https://godoc.org/sigs.k8s.io/controller-runtime/pkg/client/fake),
+but we generally reccomend using


s/reccomend/recommend/

thbkrkr · 2019-02-05T10:38:53Z

FAQ.md

+  [envtest.Environment](https://godoc.org/sigs.k8s.io/controller-runtime/pkg/envtest#Environment)
+  to spin up a real API server instead of trying to mock one out.
+
+- Structure your tests to check the that the state of the world is as you


s/check the that/check that/

droot

looks good to me (there are minor nits pointed out by another reviewer).

droot · 2019-02-05T20:54:40Z

FAQ.md

+from the API groups that it needs (be they Kubernetes types or your own).
+See the [scheme builder
+docs](https://godoc.org/sigs.k8s.io/controller-runtime/pkg/scheme) for
+more information.


This is excellent. We should add these to kubebuilder book.

droot · 2019-02-05T20:58:06Z

alias.go

@@ -25,7 +25,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/runtime/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/scheme"


I am guessing this might impact scaffolded code.

Note to self: Audit scaffolded code (and figure out if migration guide needs to be written for cr-0.2.0) and update it to use cr-0.2.0.

droot · 2019-02-05T20:59:34Z

pkg/scheme/scheme.go

+func (bld *Builder) Build() (*runtime.Scheme, error) {
+	s := runtime.NewScheme()
+	return s, bld.AddToScheme(s)
+}


thbkrkr · 2019-02-06T16:17:56Z

FAQ.md

+general, your application should have its own Scheme containing the types
+from the API groups that it needs (be they Kubernetes types or your own).
+See the [scheme builder
+docs](https://godoc.org/sigs.k8s.io/controller-runtime/pkg/scheme) for


s|controller-runtime/pkg/scheme|controller-runtime/pkg/runtime/scheme|

see above changes

This adds a series of FAQs. A decent amount of this should be migrated to the KB book.

I have a habit of spelling it with 2 'c's and one 'm'.

mengqiy · 2019-02-13T19:01:44Z

/lgtm
/hold
Moving packages is actually breaking changes. Maybe we should change the commit messages to use ⚠️ instead of ✨

mengqiy · 2019-02-13T19:11:27Z

IIRC we are not going to release again in release-0.1 branch, so using ⚠️ or ✨ doesn't matter much ATM :)
/hold cancel

DirectXMan12 · 2019-02-14T22:14:19Z

The package moves left aliases in place, so they shouldn't actually be breaking yet.

grantr · 2019-03-01T18:39:10Z

pkg/manager/manager.go

-	// GetRecorder returns a new EventRecorder for the provided name
-	GetRecorder(name string) record.EventRecorder
+	// GetEventRecorderFor returns a new EventRecorder for the provided name
+	GetEventRecorderFor(name string) record.EventRecorder


This is a breaking change. Was that intentional? If not, can we add the GetRecorder method back and deprecate it?

That was marked as breaking (on the commit). Did it make it into a stable branch? We should do a patch to the stable branch to restore it if so

Doesn't look like it made it into a stable branch (it's not on release-0.1). Master is intended to have breaking changes, and yes, this was intended to be breaking (it brings manager in line with other things that provide a recorder).

Oops, I missed the breaking mark (I wasn't aware of the meaning of ⚠️), so please ignore my comment. Thanks for explaining @DirectXMan12!

Enhancement on Yesno()

DirectXMan12 added 2 commits January 15, 2019 17:26

Better docs for pkg/client

4f65a85

This lays out package docs for the client package, clarifies that client.New creates a new direct-to-API client, and adds an example for indexers.

Add overview docs

02c8fb4

This adds an overview of the library to the root package.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2019

k8s-ci-robot requested review from droot and pwittrock January 16, 2019 06:41

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 16, 2019

droot reviewed Jan 17, 2019

View reviewed changes

DirectXMan12 changed the title ~~[WIP] 📖 Add project overview to docs, clean up docs~~ [WIP] 📖 Add project overview to docs, clean up docs, structure Jan 17, 2019

DirectXMan12 force-pushed the docs/overview-and-fixup branch from 4fc3d07 to 436eac7 Compare January 31, 2019 23:36

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2019

DirectXMan12 changed the title ~~[WIP] 📖 Add project overview to docs, clean up docs, structure~~ 📖 Add project overview to docs, clean up docs, structure Jan 31, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 31, 2019

DirectXMan12 added 4 commits February 4, 2019 15:38

Clean up docs

762715b

This cleans up the existing docs, clarifying working, improving spacing, and making sure all packages have docs.

⚠️ make manager a recorder.Provider

68ec58f

Manager previously conformed to a slightly different interface than recorder.Provider. This fixes that.

✨ mv pkg/{runtime,manager}/signals

484078a

pkg/runtime is a bit of a odd collection right now. This starts to move stuff out of it, leaving aliases in place instead. Since signals are used to control managers, this functionality now lives in a subpackage of manager.

✨ mv pkg/{runtime,}/signals

1fa9d06

Continue moving stuff out of pkg/runtime. the schemebuilder now lives in pkg/scheme.

DirectXMan12 force-pushed the docs/overview-and-fixup branch from 436eac7 to d5457b0 Compare February 4, 2019 23:38

rajathagasthya reviewed Feb 5, 2019

View reviewed changes

thbkrkr reviewed Feb 5, 2019

View reviewed changes

droot approved these changes Feb 5, 2019

View reviewed changes

thbkrkr reviewed Feb 6, 2019

View reviewed changes

DirectXMan12 added 2 commits February 12, 2019 17:56

Add FAQs

e591b2b

This adds a series of FAQs. A decent amount of this should be migrated to the KB book.

Fix some misspellings of 'recommend'

cb0aefb

I have a habit of spelling it with 2 'c's and one 'm'.

DirectXMan12 force-pushed the docs/overview-and-fixup branch from d5457b0 to cb0aefb Compare February 13, 2019 01:58

k8s-ci-robot assigned mengqiy Feb 13, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 13, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2019

k8s-ci-robot merged commit 45d6520 into kubernetes-sigs:master Feb 13, 2019

DirectXMan12 deleted the docs/overview-and-fixup branch February 14, 2019 22:13

droot mentioned this pull request Feb 19, 2019

Clarify use of Scheme #105

Closed

grantr reviewed Mar 1, 2019

View reviewed changes

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #295 from Liujingfang1/master

a316f5e

Enhancement on Yesno()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

📖 Add project overview to docs, clean up docs, structure #295

📖 Add project overview to docs, clean up docs, structure #295

DirectXMan12 commented Jan 16, 2019 •

edited

k8s-ci-robot commented Jan 16, 2019

droot left a comment

droot Jan 17, 2019

droot Jan 17, 2019

droot Jan 17, 2019

droot Jan 17, 2019

droot Jan 17, 2019

droot Jan 17, 2019

DirectXMan12 Jan 17, 2019

rajathagasthya Feb 5, 2019

thbkrkr Feb 5, 2019

thbkrkr Feb 5, 2019

droot left a comment

droot Feb 5, 2019

droot Feb 5, 2019

droot Feb 5, 2019

thbkrkr Feb 6, 2019

DirectXMan12 Feb 13, 2019

mengqiy commented Feb 13, 2019

mengqiy commented Feb 13, 2019 •

edited

DirectXMan12 commented Feb 14, 2019

grantr Mar 1, 2019

DirectXMan12 Mar 4, 2019 •

edited

DirectXMan12 Mar 4, 2019 •

edited

grantr Mar 4, 2019

📖 Add project overview to docs, clean up docs, structure #295

📖 Add project overview to docs, clean up docs, structure #295

Conversation

DirectXMan12 commented Jan 16, 2019 • edited

k8s-ci-robot commented Jan 16, 2019

droot 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

droot 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

mengqiy commented Feb 13, 2019

mengqiy commented Feb 13, 2019 • edited

DirectXMan12 commented Feb 14, 2019

Choose a reason for hiding this comment

DirectXMan12 Mar 4, 2019 • edited

Choose a reason for hiding this comment

DirectXMan12 Mar 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Jan 16, 2019 •

edited

mengqiy commented Feb 13, 2019 •

edited

DirectXMan12 Mar 4, 2019 •

edited

DirectXMan12 Mar 4, 2019 •

edited