-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat(dashboards)!: server-side implementation of dashboards #1028
Conversation
Co-authored-by: Claire.Nicholas <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
golangci
database/repo/count_user_test.go|33 col 15| undefined: api
database/repo/list_user_test.go|55 col 15| undefined: api (typecheck)
database/user/interface.go|30 col 31| undefined: api
database/user/interface.go|32 col 31| undefined: api
database/user/interface.go|34 col 36| undefined: api
database/user/interface.go|36 col 44| undefined: api
database/user/interface.go|38 col 33| undefined: api
database/user/interface.go|40 col 47| undefined: api
database/user/interface.go|42 col 31| undefined: api
database/user/delete.go|14 col 53| undefined: api
database/user/get.go|13 col 59| undefined: api
database/user/get_name.go|14 col 69| undefined: api
database/user/get_name.go|14 col 69| too many errors (typecheck)
mock/server/user.go|55 col 13| undefined: api
mock/server/user.go|77 col 11| undefined: api
mock/server/user.go|87 col 11| undefined: api
mock/server/user.go|111 col 11| undefined: api
mock/server/user_test.go|12 col 14| undefined: api (typecheck)
queue/redis/redis_test.go|138 col 11| undefined: api (typecheck)
scm/github/access.go|15 col 52| undefined: api
scm/github/access.go|83 col 53| undefined: api
scm/github/access.go|145 col 63| undefined: api
scm/github/access.go|189 col 62| undefined: api
scm/github/authentication.go|57 col 113| undefined: api
scm/github/authentication.go|98 col 76| undefined: api
scm/github/changeset.go|16 col 52| undefined: api
scm/github/changeset.go|45 col 54| undefined: api
scm/github/deployment.go|17 col 56| undefined: api
scm/github/deployment.go|58 col 61| undefined: api
scm/github/deployment.go|58 col 61| too many errors) (typecheck)
scm/github/access.go|15 col 52| undefined: api
scm/github/access.go|83 col 53| undefined: api
scm/github/access.go|145 col 63| undefined: api
scm/github/access.go|189 col 62| undefined: api
scm/github/authentication.go|57 col 113| undefined: api
scm/github/authentication.go|98 col 76| undefined: api
scm/github/changeset.go|16 col 52| undefined: api
scm/github/changeset.go|45 col 54| undefined: api
scm/github/deployment.go|17 col 56| undefined: api
scm/github/deployment.go|58 col 61| undefined: api
scm/github/deployment.go|58 col 61| too many errors) (typecheck)
scm/github/access.go|15 col 52| undefined: api
scm/github/access.go|83 col 53| undefined: api
scm/github/access.go|145 col 63| undefined: api
scm/github/access.go|189 col 62| undefined: api
scm/github/authentication.go|57 col 113| undefined: api
scm/github/authentication.go|98 col 76| undefined: api
scm/github/changeset.go|16 col 52| undefined: api
scm/github/changeset.go|45 col 54| undefined: api
scm/github/deployment.go|17 col 56| undefined: api
scm/github/deployment.go|58 col 61| undefined: api
scm/github/deployment.go|58 col 61| too many errors) (typecheck)
scm/github/access.go|15 col 52| undefined: api
scm/github/access.go|83 col 53| undefined: api
scm/github/access.go|145 col 63| undefined: api
scm/github/access.go|189 col 62| undefined: api
scm/github/authentication.go|57 col 113| undefined: api
scm/github/authentication.go|98 col 76| undefined: api
scm/github/changeset.go|16 col 52| undefined: api
scm/github/changeset.go|45 col 54| undefined: api
scm/github/deployment.go|17 col 56| undefined: api
scm/github/deployment.go|58 col 61| undefined: api
scm/github/deployment.go|58 col 61| too many errors (typecheck)
database/dashboard/create.go|3 col 1| directive //nolint:dupl // ignore similar code in update.go
is unused for linter "dupl" (nolintlint)
database/dashboard/update.go|3 col 1| directive //nolint:dupl // ignore similar code with create.go
is unused for linter "dupl" (nolintlint)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1028 +/- ##
=======================================
Coverage ? 65.99%
=======================================
Files ? 383
Lines ? 12309
Branches ? 0
=======================================
Hits ? 8123
Misses ? 3664
Partials ? 522
|
// ListBuildsForDashboardRepo gets a list of builds by repo ID from the database. | ||
// | ||
//nolint:lll // ignore long line length due to variable names | ||
func (e *engine) ListBuildsForDashboardRepo(ctx context.Context, r *api.Repo, branches, events []string) ([]*library.Build, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there an advantage to having this version of ListBuildsForRepo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the ListBuildsForRepo uses the map filter Gorm function, which to my knowledge doesn't cover the IN
keyword, but I could be wrong. Gorm docs are not the most intuitive. Second, I didn't want to have the created constraints baked in... as well as the pagination
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tangential: lll
has been turned off, so you shouldn't need that //nolint
directive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, cool.
}, | ||
{ | ||
"id": 1, | ||
"name": "OctoKitty", | ||
"token": null, | ||
"favorites": ["github/octocat"], | ||
"active": true, | ||
"admin": false | ||
"admin": false, | ||
"dashboards": [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"dashboards": [] | |
"dashboards": [] |
@@ -23,7 +23,8 @@ const ( | |||
"token": null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we going to need dashboard mocks?
Executing on a concept where users can create pages wherein they can look at a collection of repositories at a high level. Below is a simple mock up of the eventual UI:
This back-end struct design accounts for a few key features: share-ability, admin access, and customizability.
Share-ability: using a uniquely generated UUID that will serve as the
link
to the dashboard along with user-specificuser.Dashboards
we will be able to allow open navigation to any dashboard via a link + easy navigation through the UI via "My Dashboards", much like many group-board applications such as Draw IO, Miro, etc.Admin Access: since the dashboards will be public and discoverable, it's important to limit update/delete access. This can be done via the
Dashboard.Admins
field, which holds a slice of user_ids capable of editing the dashboard. There is also auditing in place in the form ofcreated_at
,created_by
,updated_at
,updated_by
.Customizability: choosing to store the dashboard repo collection as a JSON field allows users to customize their "subscription" to that repo, using the
Events
andBranches
field. While it would have been possible to reference repositories in a slice field of repo_ids, that way makes customization to this degree much more challenging.