Moves k8s and conduit client code to /pkg by pcalcado · Pull Request #103 · linkerd/linkerd2

pcalcado · 2018-01-02T21:03:53Z

Before I add Kubernetes API checks for the status command as part of #92 I'd like to start moving this shared code to the more standard /pkg directory

klingerf

Moving these files to a shared location makes sense to me. In the projects that I've worked with that have a top-level pkg dir, the organization of that dir always seems (to me) to be a bit inscrutable. It's usually not clear which packages belong in the dir, and the naming of the packages doesn't give a lot of context.

To help make sure we don't run into this, I think we should be really strict about naming, and we should move all shared packages into pkg. I've commented below with a few suggestions. I think we should also get rid of all of these util pacakges, and move them into better-named pkg directories: controller/api/util, web/util, controller/util.

klingerf · 2018-01-02T21:14:59Z

cli/cmd/root.go

+
 	pb "github.com/runconduit/conduit/controller/gen/public"
+	"github.com/runconduit/conduit/pkg/k8s"
 	"github.com/spf13/cobra"


I think your sorting is slightly off here -- if you remove the blank line on line 7, then the pkg/conduit import should be grouped together with the pkg/k8s import.

klingerf · 2018-01-02T21:16:38Z

controller/api/public/server.go

 	"net/http"

+	"github.com/runconduit/conduit/pkg/conduit"
+


Same comment here -- this import should be sorted into the list below it.

klingerf · 2018-01-02T21:17:45Z

pkg/conduit/client.go

 	"net/url"

+	"github.com/runconduit/conduit/pkg/k8s"
+


Sorting issue here as well.

klingerf · 2018-01-02T21:20:01Z

web/main.go

-	"github.com/runconduit/conduit/controller/api/public"
-	"github.com/runconduit/conduit/web/srv"
+	"github.com/runconduit/conduit/pkg/conduit"
+


And one more sorting issue here.

klingerf · 2018-01-02T21:29:58Z

pkg/conduit/client.go

@@ -1,4 +1,4 @@
-package public
+package conduit


I'm a bit worried that a pkg/conduit directory is going to turn into a dumping ground. Can we call this something more specific? Maybe pkg/apiclient?

On a related note, I have mixed feelings about separating controller/api/public/client.go and controller/api/public/server.go into separate packages. They represent two sides of our custom protobuf-over-http setup, so I think it makes sense to group them together.

How would you feel about renaming this package to api, and moving controller/api/public/server.go into it as well?

I think you are right about the coupling. I was originally worried about not having "too smart" client libraries, but ultimately this lib is smart because it abstracts gRPC over HTTP and extension points. I moved it back in latest commits.

klingerf · 2018-01-02T21:34:03Z

cli/cmd/status.go

@@ -8,8 +8,8 @@ import (

 	"github.com/runconduit/conduit/cli/healthcheck"


I think it makes sense to move the cli/healthcheck package to pkg/healthcheck as well, since it's imported by some of the files that you're moving to pkg/k8s.

And I'd vote to move the cli/shell package to pkg/shell as well.

klingerf · 2018-01-02T23:29:32Z

cli/cmd/install.go


 	"github.com/runconduit/conduit/controller"
-	uuid "github.com/satori/go.uuid"
+	"github.com/satori/go.uuid"


I don't think this is documented anywhere, but my preference is to explicitly set the package name when the last segment of the import path doesn't match the name of the package being imported.

siggy

overall i like the direction this is going. echoing some of kevin's comments, i'd like to understand/establish guiding principals on where things should go. a few questions along these lines:

should anything without a main go under /pkg?
when to put something under controller/k8s vs. /pkg/k8s? is it strictly a matter of whether the code is shared? is it easier to reason about if all the k8s code is in one place?
building on the previous question: does it make sense to make a distinction between library code that is shared across mains vs. single-use? when browsing the repo, i'd prefer to see all library code in one place.

As it is more commonly used in the codebase Signed-off-by: Phil Calcado <[email protected]>

Signed-off-by: Phil Calcado <[email protected]>

…rnal libs Signed-off-by: Phil Calcado <[email protected]>

Signed-off-by: Phil Calcado <[email protected]>

pcalcado · 2018-01-04T08:10:19Z

@klingerf Agree on being strict. Actually, it's not about being strict, to me it is making sure that everyone contributing to this project has a way to answer the question of "where does this module go?" without being blocked.

@siggy those are great questions and I have a suggestion on how to deal with this—hint, it has to do with not considering a go package an atomic module. I volunteer to open a PR with rationale, also dealing with @klingerf comments, just after this. wdyt?

Signed-off-by: Phil Calcado <[email protected]>

pcalcado · 2018-01-04T08:18:37Z

I think I've addressed all feedback. I'm still unsure about https://github.com/runconduit/conduit/pull/103/files#diff-a3991410d9e729a648de0e63d3f91d03R159 but couldn't track who to ask.

siggy

pcalcado added area/cli area/controller refactoring area/web labels Jan 2, 2018

pcalcado self-assigned this Jan 2, 2018

pcalcado requested review from adleong, franziskagoltz, klingerf, rmars and siggy January 2, 2018 21:03

pcalcado force-pushed the phil/conduit-status-kubeapi branch from 5b4ab3f to 942aae3 Compare January 2, 2018 21:05

klingerf reviewed Jan 2, 2018

View reviewed changes

siggy reviewed Jan 3, 2018

View reviewed changes

pcalcado force-pushed the phil/conduit-status-kubeapi branch from e5e0b25 to d45f61f Compare January 4, 2018 08:09

Phil Calcado added 8 commits January 4, 2018 00:09

Rename constructor functions from MakeXyz to NewXyz

832a32f

As it is more commonly used in the codebase Signed-off-by: Phil Calcado <[email protected]>

Make Conduit client depend on KubernetesAPI

3e91564

Signed-off-by: Phil Calcado <[email protected]>

Move Conduit client and k8s logic to standard go package dir for inte…

fce4d99

…rnal libs Signed-off-by: Phil Calcado <[email protected]>

Move dependencies to /pkg

44230d3

Signed-off-by: Phil Calcado <[email protected]>

Make conduit client more testable

8c95d2d

Signed-off-by: Phil Calcado <[email protected]>

Remove unused config object

1281792

Signed-off-by: Phil Calcado <[email protected]>

Add more test cases for marhsalling

1e634bd

Signed-off-by: Phil Calcado <[email protected]>

Move client back to controller

eeb1d50

Signed-off-by: Phil Calcado <[email protected]>

pcalcado force-pushed the phil/conduit-status-kubeapi branch from d45f61f to 372b558 Compare January 4, 2018 08:09

pcalcado force-pushed the phil/conduit-status-kubeapi branch from 372b558 to b3e32fe Compare January 4, 2018 08:14

Sort imports

13de485

Signed-off-by: Phil Calcado <[email protected]>

pcalcado force-pushed the phil/conduit-status-kubeapi branch from b3e32fe to 13de485 Compare January 4, 2018 08:16

siggy approved these changes Jan 4, 2018

View reviewed changes

pcalcado merged commit 709de5a into master Jan 4, 2018

klingerf mentioned this pull request Jan 4, 2018

Extract and test protobuf-over-http code #106

Closed

pcalcado mentioned this pull request Jan 4, 2018

Document source modules structure in Conduit project #107

Closed

pcalcado deleted the phil/conduit-status-kubeapi branch February 5, 2018 22:21

		@@ -8,8 +8,8 @@ import (

		"github.com/runconduit/conduit/cli/healthcheck"

Conversation

pcalcado commented Jan 2, 2018

Uh oh!

klingerf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

klingerf Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

pcalcado commented Jan 4, 2018

Uh oh!

pcalcado commented Jan 4, 2018

Uh oh!

siggy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

klingerf Jan 2, 2018 •

edited

Loading