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.

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

Merged
merged 23 commits into from
Apr 17, 2024

Conversation

ecrupper
Copy link
Contributor

@ecrupper ecrupper commented Dec 19, 2023

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:

dashboard_mock

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-specific user.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 of created_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 and Branches 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.

database/integration_test.go Show resolved Hide resolved
database/integration_test.go Show resolved Hide resolved
database/integration_test.go Show resolved Hide resolved
database/integration_test.go Show resolved Hide resolved
database/integration_test.go Show resolved Hide resolved
database/integration_test.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a 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)

compiler/registry/registry.go Outdated Show resolved Hide resolved
compiler/engine.go Outdated Show resolved Hide resolved
api/webhook/post.go Outdated Show resolved Hide resolved
compiler/engine.go Outdated Show resolved Hide resolved
compiler/engine.go Outdated Show resolved Hide resolved
database/user/get_name.go Outdated Show resolved Hide resolved
database/repo/interface.go Outdated Show resolved Hide resolved
database/repo/interface.go Outdated Show resolved Hide resolved
database/repo/count_user.go Outdated Show resolved Hide resolved
database/repo/list_user.go Outdated Show resolved Hide resolved
@ecrupper ecrupper marked this pull request as ready for review April 12, 2024 19:22
@ecrupper ecrupper requested a review from a team as a code owner April 12, 2024 19:22
database/dashboard/update_test.go Outdated Show resolved Hide resolved
database/dashboard/update_test.go Show resolved Hide resolved
database/dashboard/create.go Show resolved Hide resolved
database/dashboard/create.go Outdated Show resolved Hide resolved
database/dashboard/update.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Apr 12, 2024

Codecov Report

Attention: Patch coverage is 87.43316% with 47 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (main@52c741c). Click here to learn what that means.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1028   +/-   ##
=======================================
  Coverage        ?   65.99%           
=======================================
  Files           ?      383           
  Lines           ?    12309           
  Branches        ?        0           
=======================================
  Hits            ?     8123           
  Misses          ?     3664           
  Partials        ?      522           
Files Coverage Δ
api/types/dashboard.go 100.00% <100.00%> (ø)
api/types/dashboard_repo.go 100.00% <100.00%> (ø)
api/types/events.go 100.00% <ø> (ø)
api/types/user.go 100.00% <100.00%> (ø)
database/dashboard/delete.go 100.00% <100.00%> (ø)
database/dashboard/table.go 100.00% <100.00%> (ø)
database/database.go 58.00% <ø> (ø)
database/user/table.go 100.00% <ø> (ø)
database/user/user.go 86.11% <100.00%> (ø)
mock/server/user.go 0.00% <ø> (ø)
... and 9 more

database/dashboard/create.go Outdated Show resolved Hide resolved
database/dashboard/update.go Outdated Show resolved Hide resolved
api/dashboard/update.go Outdated Show resolved Hide resolved
database/dashboard/create.go Outdated Show resolved Hide resolved
database/dashboard/create.go Outdated Show resolved Hide resolved
database/dashboard/create.go Outdated Show resolved Hide resolved
// 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) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah okay, cool.

api/dashboard/create.go Outdated Show resolved Hide resolved
api/dashboard/create.go Outdated Show resolved Hide resolved
api/dashboard/list_user.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
database/dashboard/dashboard.go Outdated Show resolved Hide resolved
},
{
"id": 1,
"name": "OctoKitty",
"token": null,
"favorites": ["github/octocat"],
"active": true,
"admin": false
"admin": false,
"dashboards": []
Copy link
Contributor

@plyr4 plyr4 Apr 17, 2024

Choose a reason for hiding this comment

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

Suggested change
"dashboards": []
"dashboards": []

@@ -23,7 +23,8 @@ const (
"token": null,
Copy link
Contributor

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?

@ecrupper ecrupper merged commit 66b3088 into main Apr 17, 2024
14 of 16 checks passed
@ecrupper ecrupper deleted the feat/dashboard/poc branch April 17, 2024 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants