Skip to content

Improve project structure and documentation (package docs, use internal packages) #1405

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.

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

Closed
Tracked by #1395
mafredri opened this issue May 12, 2022 · 8 comments
Closed
Tracked by #1395
Labels
api Area: HTTP API docs Area: coder.com/docs needs decision Needs a higher-level decision to be unblocked.

Comments

@mafredri
Copy link
Member

mafredri commented May 12, 2022

I was inspired by #1395 and #1398 to take a look at our current go docs.

A few observations:

  • Most packages are lacking documentation (we should add the // Package [name] ... header to those)
  • Many test packages are public and can be imported by others as a dependency (we don't want to allow our users to easily break their workflows by exposing APIs that break on a whim)
    • By moving these to a path named internal, only coder/coder can import them
    • Renaming these has the added benefit of decreasing API/docs surface and makes it clear they are not public
  • Quite a few types are lacking documentation
    • Example: agent.{Metadata,Options}, it's a good practice for all types to have documentation, even if their purpose is quite clear. It's also a good chance to document edge cases and behaviors, if applicable

I'm proposing the following changes (in addition to writing more package docs):

  • buildinfo => internal/buildinfo
    • Motivation: This package only contains useful information when coder is built via project build scripts (data injected at build time), anyone importing this package will see empty values
  • (maybe) cli => internal/cli
    • Note: Not necessarily the sub-packages
    • Motivation: We only export Root() which is only useful if someone wants the same functionality as the cmd/coder cli (I don't think we want anyone using this in their project(s))
  • cli/clitest => internal/clitest:
    • Motivation: Only we should be testing that our cli commands behave as expected
  • coderd/coderdtest => internal/coderdtest
    • Motivation: Only we should be testing that our APIs behave as expected, we don't want our users to accidentally become dependent on functionality in this package
  • coderd/database/databasefake => internal/databasefake
    • Motivation: Only used for testing, not advisable to be used in production
  • (maybe) provisioner/echo => internal/provisioner/echo
    • Motivation: Are there legit use cases for others to use this, or test-only? If there are no use-cases we may not want to expose this before we know how it will or should be used
  • pty/ptytest => internal/ptytest
    • Motivation: Test helper, we don't want others to depend on this package

I mostly took a surface look approach to this, there may be other non-obvious (to me) packages that could be hidden away.

No exploration was done for hiding away package functions and types, but we may want to consider that as well. The fewer things we export, the clearer the project and it's capabilities are.

To view the docs yourself, you can use godoc:

go install golang.org/x/tools/cmd/godoc@latest
cd /path/to/coder
godoc -http=:6060
# open http://localhost:6060/pkg/github.com/coder/coder/
@mafredri mafredri added docs Area: coder.com/docs api Area: HTTP API needs grooming 🪒 labels May 12, 2022
@mafredri mafredri changed the title Improve project structure and documentation (package docs, use internal package) Improve project structure and documentation (package docs, use internal packages) May 12, 2022
@greyscaled
Copy link
Contributor

greyscaled commented May 12, 2022

@mafredri and @dwahler - I think we can add this to the docs epic (#1395) , wdyt ?

@tjcran
Copy link

tjcran commented May 16, 2022

Totally fine if this gets picked up in some spare cycles, but it seems like this is just a nice to have for the launch

@Emyrk
Copy link
Member

Emyrk commented May 18, 2022

We should move all of coderd into /internal. No?

@mafredri
Copy link
Member Author

@Emyrk I think that's an option, but not a necessity. I also think that's too big an architectural change for this issue. I.e. we'd need to re-evaluate what we want to expose and what we don't. How do we want others to see the project/package? The packages originally suggested here should be sufficiently self-contained for a cleanup job. I believe @kylecarbs might also have some thoughts on why we might not want to move coderd into internal?

@deansheather
Copy link
Member

I'm against using internal because I've been annoyed by open source repos on GitHub putting useful functions/types inside internal and imports being blocked by the compiler.

@mafredri
Copy link
Member Author

I can definitely relate to that @deansheather. IMO there's a balance to be had there. On the one hand, the Go mantra of "a little copying is better than a little dependency" is good to keep in mind here, things are usually made internal because if someone is depending on that API, they're in for a bad time (API breakage). This also means that anything that is made public becomes harder to mutate, or risk breaking dependents.

@misskniss misskniss added the needs decision Needs a higher-level decision to be unblocked. label May 31, 2022
@misskniss misskniss modified the milestones: Community MVP, Post-MVP Jun 1, 2022
@coadler
Copy link
Contributor

coadler commented Jun 1, 2022

A personal anecdote: I was browsing the Tailscale repo today and found their flat structure extremely easy to consume.

@bpmct
Copy link
Member

bpmct commented Oct 26, 2022

Gonna close for now until we have a more concrete plan.

@bpmct bpmct closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Area: HTTP API docs Area: coder.com/docs needs decision Needs a higher-level decision to be unblocked.
Projects
None yet
Development

No branches or pull requests

10 participants