Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

chore/msp-example: refactor to align with service structure best practices#62954

Merged
bobheadxi merged 1 commit into
mainfrom
msp-example-refactor
May 29, 2024
Merged

chore/msp-example: refactor to align with service structure best practices#62954
bobheadxi merged 1 commit into
mainfrom
msp-example-refactor

Conversation

@bobheadxi

@bobheadxi bobheadxi commented May 28, 2024

Copy link
Copy Markdown
Member

In monorepo:

cmd/my-service/main.go
-> cmd/my-service/service
-> cmd/my-service/internal/...

Outside monorepo a similar unnested structure aligns with this as well:

cmd/my-service <- command
service/...    <- entrypoint
internal/...   <- internal implementation

Test plan

Basic example builds and runs: sg run msp-example

@bobheadxi bobheadxi requested review from a team and unknwon May 28, 2024 22:13
@cla-bot cla-bot Bot added the cla-signed label May 28, 2024
Comment thread cmd/msp-example/main.go
"github.com/sourcegraph/sourcegraph/lib/managedservicesplatform/runtime"

"github.com/sourcegraph/sourcegraph/cmd/msp-example/internal/example"
"github.com/sourcegraph/sourcegraph/cmd/msp-example/service"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was kinda expecting this?

Suggested change
"github.com/sourcegraph/sourcegraph/cmd/msp-example/service"
"github.com/sourcegraph/sourcegraph/cmd/msp-example/internal/service"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I thought about that but I think it's a top-level construct and I want to avoid nesting too much, perhaps. That way internal/foobar still has meaning and it's clear it's not on the same level as internal/service

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That way internal/foobar still has meaning and it's clear it's not on the same level as internal/service

I think that's not a useful "clear" signal than "service" is internal to the MSP service, should not be used by any others 🤔

@bobheadxi bobheadxi May 29, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The way I think about it, "service" is effectively the entrypoint to all the stuff in internal/, i.e.

Command -> Service -> Internals

So the exported Service isn't useful in practice, but in terms of hierarchy I think it has an important place as "not just another internal package"

@unknwon unknwon May 29, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK I think it just a bit weird in a monorepo setup, where cmd/ is the top-level directory that includes everything.

Looking at on SAMS repo, for example.

cmd/accounts-servver imports backend/service seems making sense, and actually required because cmd can't import backend/internal/service.

@bobheadxi bobheadxi May 29, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, for a standalone repo case it would be:

cmd/my-service <- command
service/...    <- entrypoint
internal/...   <- internal implementation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All good, let's go!

@bobheadxi bobheadxi merged commit 17a5fdb into main May 29, 2024
@bobheadxi bobheadxi deleted the msp-example-refactor branch May 29, 2024 16:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants