Skip to content
This repository was archived by the owner on Nov 7, 2022. It is now read-only.

Repackage collector to fit go project layout guidelines #430

Closed
wants to merge 1 commit into from
Closed

Repackage collector to fit go project layout guidelines #430

wants to merge 1 commit into from

Conversation

sjkaris
Copy link

@sjkaris sjkaris commented Feb 21, 2019

This contains NO CODE CHANGES.

Repackage project to better fit https://github.com/golang-standards/project-layout.

Additional refactoring is required, as anything in /pkg/ must not import anything in /internal/.

Changes:
/processor moved into /pkg/
/receiver moved into /pkg/
/translator moved into /pkg/
/exporter moved into /pkg/
/cmd/occollector/app/sender moved into /internal/

Reasoning:
/pkg refactor: Conceptually, we want to expose processor, receiver, and exporter interfaces to the outside world, and each of the implementations of these components should be standalone and usable without the greater project. Thus, I thought /pkg/ was a good fit for these components. Translator, although it does not contain interfaces, is used by the other packages that are moved into /pkg, thus I thought it also fit well under /pkg.
/cmd/occollector/app/sender refactor: /cmd should contain minimal code and really just deal with piecing the components together to form the final runnable binary. This should be easily replaceable by users wishing to form smaller/trimmed down executables.

@sjkaris sjkaris requested a review from a team as a code owner February 21, 2019 21:13
@sjkaris
Copy link
Author

sjkaris commented Feb 21, 2019

This is also more of a proposal to repackage, very happy to take suggestions, or forgo this refactor altogether.

@sjkaris sjkaris requested a review from songy23 February 21, 2019 21:30
@flands flands added this to the 0.2.0 milestone Feb 21, 2019
@odeke-em
Copy link
Member

Hello @sjkaris!

/pkg refactor: Conceptually, we want to expose processor, receiver, and exporter interfaces to the outside world, and each of the implementations of these components should be standalone and usable without the greater project. Thus, I thought /pkg/ was a good fit for these components. Translator, although it does not contain interfaces, is used by the other packages that are moved into /pkg, thus I thought it also fit well under /pkg.
/cmd/occollector/app/sender refactor: /cmd should contain minimal code and really just deal with piecing the components together to form the final runnable binary. This should be easily replaceable by users wishing to form smaller/trimmed down executables.

Am not the biggest fan of putting things under pkg, in my humble opinion that defeats Go style structuring where everything is already a package of its own that can be called by the outside world, unless it is under "internal" of which we already do have. This new project style is used by Kubernetes, Jaeger but unfortunately I've not seen it recommended in other places like official Go packages.

Anyways that's my opinion, this structure can be okay but I think that the reasoning of exposing components as standalone already applies as is today.

@pjanotti
Copy link

If we take the organization to its full consequence then we should have 3 folders at the repo root containing go sources:

  1. ./cmd
  2. ./pkg
  3. ./internal

I like that, but it isn't a big practical difference. What I think it is a concrete difference is that it express our intent of "supporting" what is under pkg.

btw, I suggest to move ./example to ./cmd/example (then later we can have even more examples)

@sjkaris
Copy link
Author

sjkaris commented Feb 26, 2019

+1 to moving examples.

Yeah, I like the signalling of having /pkg to indicate our intent. Sure, anyone can load whatever package (even internal) but having the convention I think helps let people know what they should and shouldn't be re-using. I also think it helps make it clear that things in /pkg shouldn't use things in /internal for example, whereas currently its not clear that /exporters shouldn't use /internal.

@flands flands modified the milestones: 0.1.3, 0.1.4 Feb 27, 2019
Copy link
Contributor

@songy23 songy23 left a comment

Choose a reason for hiding this comment

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

Took a look at K8S Go client. They have /pkg but looks like they still have other top-level packages that they're supporting: https://github.com/kubernetes/client-go#whats-included.

What I think it is a concrete difference is that it express our intent of "supporting" what is under pkg.

This sounds reasonable to me, though I'm less sure if this is a convention in Go packaging.

@bogdandrutu
Copy link

Got some feedback from @rakyll and also looked at that repo that you pointed (which seems to be a one person work and not the official repo). See this https://rakyll.org/style-packages/ and search for "Clean import paths"

@sjkaris
Copy link
Author

sjkaris commented Feb 27, 2019

@bogdandrutu its not an official thing certainly, although it appears relatively popular and both kubernetes and jaeger seem to mostly follow it. Currently we seem to half-implement it. I also think having some rules / structure would be helpful here, and I think an additional grouping here would go a long way in helping us define what should go where. While I do like the idea of having a similar project structure to other popular go codebases, I'm not particularly tied to the name pkg and we can rename that to something like module or plugin or something since that mostly achieves the same thing.

@pjanotti
Copy link

We don't seem to have reached an agreement on this, let's close this PR and perhaps revisit this issue later.

@pjanotti pjanotti closed this Mar 13, 2019
@rakyll
Copy link
Contributor

rakyll commented Mar 13, 2019

Please see golang-standards/project-layout#10.

# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants