Skip to content

[common-go] Move db models to common-go #14731

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
wants to merge 2 commits into from
Closed

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Nov 16, 2022

Description

Moves DB models to common-go, such that they can be used by public-api but also usage. Ideally, this wouldn't be in a common package but right now this is not possible as we have DB logic spread across multiple systems. Sharing through common-go is better than duplicating, as that would drift quickly.

Changes performed in this PR:

  • Move all DB models from components/usage/pkg/db into components/common-go/db
  • Update packages correspondingly
  • Update imports correspondingly
  • Update leeway BUILD definition such that common-go depends on gitpod-protocol for starting the DB
  • Removes dependency from usage on gitpod-protocol for starting the DB
  • Updates go.mod/go.sum

Related Issue(s)

Fixes #

How to test

CI builds

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@easyCZ easyCZ force-pushed the mp/common-move-db-models branch 2 times, most recently from 9d3e306 to 65abd99 Compare November 16, 2022 10:42
@easyCZ easyCZ marked this pull request as ready for review November 16, 2022 10:44
@easyCZ easyCZ requested review from a team November 16, 2022 10:44
@github-actions github-actions bot added team: SID team: IDE team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Nov 16, 2022
@easyCZ
Copy link
Member Author

easyCZ commented Nov 16, 2022

/werft run

👍 started the job as gitpod-build-mp-common-move-db-models.6
(with .werft/ from main)

@easyCZ easyCZ force-pushed the mp/common-move-db-models branch from 743c10b to 12466eb Compare November 16, 2022 11:31
@easyCZ
Copy link
Member Author

easyCZ commented Nov 16, 2022

/werft run

👍 started the job as gitpod-build-mp-common-move-db-models.10
(with .werft/ from main)

@mads-hartmann
Copy link
Contributor

@easyCZ I think the Leeway build errors are due to a bug in Leeway. Go packages check that all transitive dependencies exist in the cache (here) but for whatever reason, components/common-go:init-testdb hasn't been build yet when it tries to build the other packages (e.g. [components/content-service:lib] Reason: package "components/common-go:init-testdb" is not built, from this job).

Reading through Leeway I see that for Yarn packages it skips ephemeral packages when checking transitive dependencies. I wonder if we need to do something similar for Go packages. I will follow up with the team.

@easyCZ easyCZ force-pushed the mp/common-move-db-models branch 2 times, most recently from 3b87ac8 to 6310847 Compare November 16, 2022 13:45
@easyCZ
Copy link
Member Author

easyCZ commented Nov 16, 2022

/werft run

👍 started the job as gitpod-build-mp-common-move-db-models.12
(with .werft/ from main)

@mads-hartmann
Copy link
Contributor

@easyCZ As a quick hack I copied over the Yarn implementation to the Go one. You could try it on your change using werft job run github -a leewayfromgit=b7ff6948e10258de9102f0012b6e733d1a929a32 and see if that fixes it. I haven't tested it, and don't even know if this is desirable so take it with a grain of salt.

@easyCZ
Copy link
Member Author

easyCZ commented Nov 16, 2022

@mads-hartmann That got me further. Still failed but with application level problems, not build problems - https://werft.gitpod-dev.com/job/gitpod-build-mp-common-move-db-models.13

@easyCZ easyCZ force-pushed the mp/common-move-db-models branch from 6310847 to 9564a27 Compare November 16, 2022 15:42
@easyCZ
Copy link
Member Author

easyCZ commented Nov 16, 2022

@mads-hartmann I've triggered https://werft.gitpod-dev.com/job/gitpod-build-mp-common-move-db-models.15 which should contain a fix for the application level.

@easyCZ
Copy link
Member Author

easyCZ commented Nov 16, 2022

https://werft.gitpod-dev.com/job/gitpod-build-mp-common-move-db-models.15 failed, but I can't tell why. In any case, it looks like that change does unblock this.

@mads-hartmann
Copy link
Contributor

mads-hartmann commented Nov 17, 2022

@easyCZ @laushinka Looking at the logs from the most recent job I see

[components/ws-daemon:docker|DONE] build succeeded
[install/installer:raw-app] level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"github.com/gitpod-io/gitpod/usage/pkg/db\""
[install/installer:raw-app] level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"github.com/gitpod-io/gitpod/usage/pkg/db\"\n\n"
[install/installer:raw-app] package build failed
[install/installer:raw-app] Reason: exit status 3
[install/installer:raw-app|FAIL] exit status 3
☁️  uploading build artifacts to remote cache
build failed
Reason: build failed
time="2022-11-16T15:50:23Z" level=fatal msg="build failed"

That might be worth looking into. In the mean time I will try to get the fix into main - I just have to talk to Chris to understand if the change is desirable.

@laushinka
Copy link
Contributor

That might be worth looking into.

Taking a look!

@csweichel
Copy link
Contributor

/hold

more discussion needed

@csweichel
Copy link
Contributor

This change repeats the gitpod-protocol mistake:

  • it turns common-go into more of a kitchen sink than it currently is
  • it muddles the dependencies on shared state (the DB)

There are different ways to move forward:

  • introduce something like a go-db component
  • finally split common-go into individual modules

@laushinka
Copy link
Contributor

laushinka commented Nov 17, 2022

  • introduce something like a go-db component

After a discussion, it's preferred to have something like a go package under /gitpod-db. I will work on the changes.

New branch[1]

@easyCZ
Copy link
Member Author

easyCZ commented Nov 18, 2022

introduce something like a go-db component
finally split common-go into individual modules

Yeah that's also a perfectly viable option.

@easyCZ
Copy link
Member Author

easyCZ commented Nov 18, 2022

Moving to draft, as we'll be adding this to a new module.

@easyCZ easyCZ marked this pull request as draft November 18, 2022 09:15
@laushinka
Copy link
Contributor

Closing this, as it is continued in #14770.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants