Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Split up DAL + Controller into packages by functionality #1567

Open
alecthomas opened this issue May 24, 2024 · 2 comments
Open

Split up DAL + Controller into packages by functionality #1567

alecthomas opened this issue May 24, 2024 · 2 comments
Assignees
@alecthomas
Labels
techdebt Issue is technical debt

Comments

@alecthomas
Copy link
Collaborator

eg. put all lease related queries + DAL into a separate backend/controller/leases package along with all of the related logic
@github-actions github-actions bot added the triage Issue needs triaging label May 24, 2024
@ftl-robot ftl-robot mentioned this issue May 24, 2024
Open
@alecthomas alecthomas added the techdebt Issue is technical debt label May 28, 2024
@wesbillman wesbillman removed the triage Issue needs triaging label May 28, 2024
@matt2e matt2e added the next Work that will be be picked up next label Jun 18, 2024
@deniseli deniseli self-assigned this Jun 20, 2024
@github-actions github-actions bot removed the next Work that will be be picked up next label Jun 20, 2024
@deniseli
Copy link
Contributor

reminder: update package comment on dalerrs.go

@deniseli deniseli mentioned this issue Jun 21, 2024
Merged
deniseli added a commit that referenced this issue Jul 8, 2024
@deniseli
chore: refactor out separate dal and sql packages for cronjobs (#…
8fedcd7 
…1860)

Part 1 of #1567: This PR
refactors `cronjobs` dal and sql packages out from the parent controller
package. I'm breaking this refactor into many PRs to combat merge
conflicts.

Ultimate plan for the refactor:
* `controller/domain/{dal,sql}` packages hold all the query logic for
each domain
* When a domain `D` contains certain use cases (i.e. functions) that
need to exec queries from a different domain `E`, `D/dal` will call
`E/dal`. If those queries spanning multiple domains need to be merged
into a single transaction, then `D/dal` will create a wrapper
transaction that gets bundled with a `qtx` for each domain that gets
queried. `CreateDeployment` is an example of this that should eventually
be refactored out of `controller/dal` into `controller/deployment/dal`.
* `controller/dal` will cease to exist.
* `controller/sql` will not contain any queries. Instead, it will house
top-level db artifacts (e.g. `sql/schema`)

In the meantime, while this refactor is in progress, `controller/dal`
will contain methods that are yet to be pulled out.

This PR does not yet generalize the top-level `Tx` logic because
`controller/dal` is still the only dal package that uses it.
@deniseli
Copy link
Contributor

deniseli commented Aug 7, 2024

Whenever we pick this back up, I'd like to split up dal.go just into separate files to start out, before we even begin the package breakdown, just to get things into a slightly more organized state first and prevent excessively-bad merge conflicts. I should have started with that all along - would have helped with anticipating what would/wouldn't work too.

@gak gak assigned alecthomas and unassigned deniseli Aug 14, 2024
This was referenced Aug 27, 2024
Open
Open
Open
Open
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alecthomas alecthomas

Labels
techdebt Issue is technical debt
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@alecthomas @wesbillman @matt2e @deniseli