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

History and Version Control for Dashboard Updates #8472

Merged
merged 4 commits into from Jun 7, 2017

Conversation

Projects
None yet
4 participants
@bentranter
Contributor

bentranter commented May 26, 2017

A simple version control system for dashboards. Closes #1504.

Goals

  1. To create a new dashboard version every time a dashboard is saved.
  2. To allow users to view all versions of a given dashboard.
  3. To allow users to rollback to a previous version of a dashboard.
  4. To allow users to compare two versions of a dashboard.

Usage

Navigate to a dashboard, and click the settings cog. From there, click the "Changelog" button to be brought to the Changelog view. In this view, a table containing each version of a dashboard can be seen.

Each entry in the table represents a dashboard version. A selectable checkbox, the version number, date created, name of the user who created that version, and commit message is shown in the table, along with a button that allows a user to restore to a previous version of that dashboard.

If a user wants to restore to a previous version of their dashboard, they can do so by clicking the previously mentioned button.

If a user wants to compare two different versions of a dashboard, they can do so by clicking the checkbox of two different dashboard versions, then clicking the "Compare versions" button located below the dashboard. From there, the user is brought to a view showing a summary of the dashboard differences. Each summarized change contains a link that can be clicked to take the user a JSON diff highlighting the changes line by line.

Overview of Changes

Backend Changes

  • A dashboard_version table was created to store each dashboard version, along with a dashboard version model and structs to represent the queries and commands necessary for the dashboard version API methods.
  • API endpoints were created to support working with dashboard versions.
  • Methods were added to create, update, read, and destroy dashboard versions in the database.
  • Logic was added to compute the diff between two versions, and display it to the user.
  • The dashboard migration logic was updated to save a "Version 1" of each existing dashboard in the database.

Frontend Changes

  • New views
  • Methods to consume API endpoints
  • Caching of diff views

New API Endpoints

GET "/api/dashboards/db/:dashboardId/versions?orderBy=<string>&limit=<int>&start=<int>"

Get all dashboard versions for the given dashboard ID. Accepts three URL parameters:

  • orderBy String to order the results by. Possible values are version, created, created_by, message. Default is versions. Ordering is always in descending order.
  • limit Maximum number of results to return
  • start Position in results to start from

GET "/api/dashboards/db/:dashboardId/versions/:id"

Get an individual dashboard version by ID, for the given dashboard ID.

POST "/api/dashboards/db/:dashboardId/restore"

Restore to the given dashboard version. Post body is of content-type application/json, and must contain:

{
	"dashboardId": <int>,
	"version": <int>
}

GET "/api/dashboards/db/:dashboardId/compare/:versionA...:versionB"

Compare two dashboard versions by ID for the given dashboard ID, returning a JSON delta formatted representation of the diff. The URL format follows what GitHub does. For example, visiting /api/dashboards/db/18/compare/22...33 will return the diff between versions 22 and 33 for the dashboard ID 18.

Dependencies Added

  • The Go package gojsondiff was added and vendored.

Notes

There are some things we're aware of that still need to be done before this should be merged (some tests for the diffing code, light styles to accompany the dark CSS styles for one of the views), but figured it'd be good to get this PR out there so Grafana and the community can start to review it.

If anyone is interested, we've got our own staging server setup here. It's a fresh Grafana installation with no dashboards yet, and has default credentials. We'll be taking that server down once this PR is done, so feel free to try anything on that staging server.

Ben Tranter and others added some commits May 24, 2017

Ben Tranter Carlos Rosquillas
History and Version Control for Dashboard Updates
A simple version control system for dashboards. Closes #1504.

Goals

1. To create a new dashboard version every time a dashboard is saved.
2. To allow users to view all versions of a given dashboard.
3. To allow users to rollback to a previous version of a dashboard.
4. To allow users to compare two versions of a dashboard.

Usage

Navigate to a dashboard, and click the settings cog. From there, click
the "Changelog" button to be brought to the Changelog view. In this
view, a table containing each version of a dashboard can be seen. Each
entry in the table represents a dashboard version. A selectable
checkbox, the version number, date created, name of the user who created
that version, and commit message is shown in the table, along with a
button that allows a user to restore to a previous version of that
dashboard. If a user wants to restore to a previous version of their
dashboard, they can do so by clicking the previously mentioned button.
If a user wants to compare two different versions of a dashboard, they
can do so by clicking the checkbox of two different dashboard versions,
then clicking the "Compare versions" button located below the dashboard.
From there, the user is brought to a view showing a summary of the
dashboard differences. Each summarized change contains a link that can
be clicked to take the user a JSON diff highlighting the changes line by
line.

Overview of Changes

Backend Changes

- A `dashboard_version` table was created to store each dashboard
  version, along with a dashboard version model and structs to represent
  the queries and commands necessary for the dashboard version API
  methods.
- API endpoints were created to support working with dashboard
  versions.
- Methods were added to create, update, read, and destroy dashboard
  versions in the database.
  - Logic was added to compute the diff between two versions, and
  display it to the user.
  - The dashboard migration logic was updated to save a "Version
  1" of each existing dashboard in the database.

Frontend Changes

- New views
- Methods to pull JSON and HTML from endpoints

New API Endpoints

Each endpoint requires the authorization header to be sent in
the format,

```
Authorization: Bearer <jwt>
```

where `<jwt>` is a JSON web token obtained from the Grafana
admin panel.

`GET "/api/dashboards/db/:dashboardId/versions?orderBy=<string>&limit=<int>&start=<int>"`

Get all dashboard versions for the given dashboard ID. Accepts
three URL parameters:

- `orderBy` String to order the results by. Possible values
  are `version`, `created`, `created_by`, `message`. Default
  is `versions`. Ordering is always in descending order.
- `limit` Maximum number of results to return
- `start` Position in results to start from

`GET "/api/dashboards/db/:dashboardId/versions/:id"`

Get an individual dashboard version by ID, for the given
dashboard ID.

`POST "/api/dashboards/db/:dashboardId/restore"`

Restore to the given dashboard version. Post body is of
content-type `application/json`, and must contain.

```json
{
  "dashboardId": <int>,
  "version": <int>
}
```

`GET "/api/dashboards/db/:dashboardId/compare/:versionA...:versionB"`

Compare two dashboard versions by ID for the given
dashboard ID, returning a JSON delta formatted
representation of the diff. The URL format follows
what GitHub does. For example, visiting
[/api/dashboards/db/18/compare/22...33](http://ec2-54-80-139-44.compute-1.amazonaws.com:3000/api/dashboards/db/18/compare/22...33)
will return the diff between versions 22 and 33 for
the dashboard ID 18.

Dependencies Added

- The Go package [gojsondiff](https://github.com/yudai/gojsondiff)
  was added and vendored.
Merge pull request #10 from walmartlabs/version-control
History and Version Control for Dashboard Updates
@torkelo

Overall this looks really good! Just a few minor things that could be improved.

Show outdated Hide outdated pkg/api/dashboard.go
for i, dashboardVersion := range query.Result {
creator := "Anonymous"
if dashboardVersion.CreatedBy > 0 {
creator = getUserLogin(dashboardVersion.CreatedBy)

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

Doing a db lookup in a loop here is not great.

  1. Either create a read only projection (you already have one in DashboardVersionDTO) and write a custom SQL read for this that inner joins on created by to get username
  2. If you feel that is too much work, using a cache here so the same user is not fetched more than once could also help a bit. But then in some cases a dashboard might have a really long history with over 40 users doing changes (seems like a rare occurrence) but that would cause 40 user db lookup SQL queries.
@torkelo

torkelo May 26, 2017

Member

Doing a db lookup in a loop here is not great.

  1. Either create a read only projection (you already have one in DashboardVersionDTO) and write a custom SQL read for this that inner joins on created by to get username
  2. If you feel that is too much work, using a cache here so the same user is not fetched more than once could also help a bit. But then in some cases a dashboard might have a really long history with over 40 users doing changes (seems like a rare occurrence) but that would cause 40 user db lookup SQL queries.
Show outdated Hide outdated pkg/api/dashboard.go
@@ -255,6 +258,264 @@ func GetDashboardFromJsonFile(c *middleware.Context) {
c.JSON(200, &dash)
}
// GetDashboardVersions returns all dashboardversions as JSON
func GetDashboardVersions(c *middleware.Context) {

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

please use the new route handler signature that returns a response, like this https://github.com/grafana/grafana/blob/master/pkg/api/apikey.go#L11

Creates more readable code as error handling clauses can simple return ApiError(500, "something", err), instead of like this where you have to call a method (c.JsonApiErr), and then return. Trying to refactor and move away from that style.

It requires that the handler is wrapped when you register the route, https://github.com/grafana/grafana/blob/master/pkg/api/api.go#L187

@torkelo

torkelo May 26, 2017

Member

please use the new route handler signature that returns a response, like this https://github.com/grafana/grafana/blob/master/pkg/api/apikey.go#L11

Creates more readable code as error handling clauses can simple return ApiError(500, "something", err), instead of like this where you have to call a method (c.JsonApiErr), and then return. Trying to refactor and move away from that style.

It requires that the handler is wrapped when you register the route, https://github.com/grafana/grafana/blob/master/pkg/api/api.go#L187

Show outdated Hide outdated pkg/api/dashboard.go
// GetDashboardVersion returns the dashboard version with the given ID.
func GetDashboardVersion(c *middleware.Context) {
dashboardIdStr := c.Params(":dashboardId")
dashboardId, err := strconv.Atoi(dashboardIdStr)

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

Is there a reason not to use the c.ParamsInt64(":dashboardId"), method here?

@torkelo

torkelo May 26, 2017

Member

Is there a reason not to use the c.ParamsInt64(":dashboardId"), method here?

Show outdated Hide outdated pkg/api/dashboard.go
c.JSON(200, dashVersionMeta)
}
func dashCmd(c *middleware.Context) (m.CompareDashboardVersionsCommand, error) {

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

this function name seems a bit undescriptive given that it's api package level static function. Maybe createCompareDashboardVersionCommand

@torkelo

torkelo May 26, 2017

Member

this function name seems a bit undescriptive given that it's api package level static function. Maybe createCompareDashboardVersionCommand

Show outdated Hide outdated pkg/api/dashboard.go
return
}
c.JSON(200, simplejson.NewFromAny(util.DynMap{

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

Is there a reason to wrap the return map in simplejson? We use it only when manipulating / constructing json (and reading)

@torkelo

torkelo May 26, 2017

Member

Is there a reason to wrap the return map in simplejson? We use it only when manipulating / constructing json (and reading)

Show outdated Hide outdated public/app/core/directives/dash_edit_link.js
@@ -8,6 +8,7 @@ function ($, coreModule) {
var editViewMap = {
'settings': { src: 'public/app/features/dashboard/partials/settings.html'},
'annotations': { src: 'public/app/features/annotations/partials/editor.html'},
'audit': { src: 'public/app/features/dashboard/audit/partials/audit.html'},

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

I think history is more fitting name for this view.

@torkelo

torkelo May 26, 2017

Member

I think history is more fitting name for this view.

Show outdated Hide outdated public/app/features/all.js
@@ -2,6 +2,7 @@ define([
'./panellinks/module',
'./dashlinks/module',
'./annotations/all',
'./annotations/annotations_srv',

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

this looks strange, as annotation_srv is included in annotations/all

@torkelo

torkelo May 26, 2017

Member

this looks strange, as annotation_srv is included in annotations/all

Show outdated Hide outdated public/app/features/dashboard/all.js
@@ -1,10 +1,12 @@
define([
'./dashboard_ctrl',
'./alerting_srv',
'./audit/audit_srv',

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

I prefer historySrv (history_srv file) and HistoryCtrl (history_ctrl file).

Think the name audit will be better suited for a future audit feature (that will be more generic for changes to users/permissions/data sources)

@torkelo

torkelo May 26, 2017

Member

I prefer historySrv (history_srv file) and HistoryCtrl (history_ctrl file).

Think the name audit will be better suited for a future audit feature (that will be more generic for changes to users/permissions/data sources)

Show outdated Hide outdated public/app/features/dashboard/audit/audit_ctrl.ts
import {DashboardModel} from '../model';
import {AuditLogOpts, RevisionsModel} from './models';
export class AuditLogCtrl {

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

DashboardHistoryListCtrl or just HistoryListCtrl

@torkelo

torkelo May 26, 2017

Member

DashboardHistoryListCtrl or just HistoryListCtrl

Show outdated Hide outdated public/app/features/dashboard/audit/partials/audit.html
@@ -0,0 +1,161 @@
<div ng-controller="AuditLogCtrl">

This comment has been minimized.

@torkelo

torkelo May 26, 2017

Member

We have for a long time been moving away from this old style of angular. Instead everything should be done as components (directives with bindToController)

https://github.com/grafana/grafana/blob/master/public/app/features/dashboard/timepicker/timepicker.ts#L170

@torkelo

torkelo May 26, 2017

Member

We have for a long time been moving away from this old style of angular. Instead everything should be done as components (directives with bindToController)

https://github.com/grafana/grafana/blob/master/public/app/features/dashboard/timepicker/timepicker.ts#L170

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Jun 1, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

sanchitraizada
Ben Tranter


Ben Tranter seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant commented Jun 1, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

sanchitraizada
Ben Tranter


Ben Tranter seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@torkelo

torkelo approved these changes Jun 5, 2017

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Jun 5, 2017

Member

this is looking close to mergable, will just refactor and change a few things (https://github.com/grafana/grafana/tree/walmartlabs-master) and maybe merge it later today or tomorrow. Then further changes & improvements can be done in new PRs.

Member

torkelo commented Jun 5, 2017

this is looking close to mergable, will just refactor and change a few things (https://github.com/grafana/grafana/tree/walmartlabs-master) and maybe merge it later today or tomorrow. Then further changes & improvements can be done in new PRs.

@@ -68,17 +69,43 @@ func SaveDashboard(cmd *m.SaveDashboardCommand) error {
}
}
affectedRows := int64(0)
parentVersion := dash.Version
version, err := getMaxVersion(sess, dash.Id)

This comment has been minimized.

@torkelo

torkelo Jun 5, 2017

Member

what was the reason for getMaxVersion? incrementing the current by one should always be safe, as if there is a version mismatch it either generates an error or if overwrite is true, the version will be taken from the current latest version, so dash.Version+1 should always be the same as getMaxVersion

@torkelo

torkelo Jun 5, 2017

Member

what was the reason for getMaxVersion? incrementing the current by one should always be safe, as if there is a version mismatch it either generates an error or if overwrite is true, the version will be taken from the current latest version, so dash.Version+1 should always be the same as getMaxVersion

This comment has been minimized.

@bentranter

bentranter Jun 7, 2017

Contributor

Yeah you're right, this is unnecessary now. I think at point during dev on this I handled the version incrementing differently which lead to cases where we needed something like this, but had since changed the logic so that it wasn't necessary anymore.

@bentranter

bentranter Jun 7, 2017

Contributor

Yeah you're right, this is unnecessary now. I think at point during dev on this I handled the version incrementing differently which lead to cases where we needed something like this, but had since changed the logic so that it wasn't necessary anymore.

}
// RestoreDashboardVersion restores a dashboard to the given version.
func RestoreDashboardVersion(c *middleware.Context, cmd m.RestoreDashboardVersionCommand) Response {

This comment has been minimized.

@torkelo

torkelo Jun 5, 2017

Member

This whole sql command seemed to duplicate a lof of existing code (Get dashboard, Get dashboard version, Save dashboard & insert history).

I refactored this to use existing queries & commands (this also made restore work for alerting, so old versions of alert rules are now also restored, as the restore logic is using the exact same code path as any dashboard save instead of duplicating it like was done below).
c87418d

@torkelo

torkelo Jun 5, 2017

Member

This whole sql command seemed to duplicate a lof of existing code (Get dashboard, Get dashboard version, Save dashboard & insert history).

I refactored this to use existing queries & commands (this also made restore work for alerting, so old versions of alert rules are now also restored, as the restore logic is using the exact same code path as any dashboard save instead of duplicating it like was done below).
c87418d

This comment has been minimized.

@bentranter

bentranter Jun 7, 2017

Contributor

Really appreciate all the work you did refactoring this PR, thank you! 🎉

@bentranter

bentranter Jun 7, 2017

Contributor

Really appreciate all the work you did refactoring this PR, thank you! 🎉

)
SELECT
dashboard.id,
dashboard.version + 1,

This comment has been minimized.

@torkelo

torkelo Jun 5, 2017

Member

This looks like a bug. The current history item should have the same version as the version in dashboard table, no?

@torkelo

torkelo Jun 5, 2017

Member

This looks like a bug. The current history item should have the same version as the version in dashboard table, no?

This comment has been minimized.

@bentranter

bentranter Jun 7, 2017

Contributor

Yeah, that was a bug, it only considered the case for new brand new dashboards that had never been changed.

@bentranter

bentranter Jun 7, 2017

Contributor

Yeah, that was a bug, it only considered the case for new brand new dashboards that had never been changed.

} else {
return this.historySrv.compareVersions(this.dashboard, compare, diff).then(response => {
this.delta[this.diff] = response;
}).catch(err => {

This comment has been minimized.

@torkelo

torkelo Jun 5, 2017

Member

error handling that shows alerts like this are not needed, it's done generically in backendSrv

@torkelo

torkelo Jun 5, 2017

Member

error handling that shows alerts like this are not needed, it's done generically in backendSrv

// A BasicDiff holds the stateful values that are used when generating a basic
// diff from JSON tokens.
type BasicDiff struct {

This comment has been minimized.

@torkelo

torkelo Jun 5, 2017

Member

Why is this formatter returning html and not the BasicDiff model? The basic diff api should retrurn this BasicDiff model, the html rendering could have been done in the frontend, what was the advantage to render this to html in the backend?

@torkelo

torkelo Jun 5, 2017

Member

Why is this formatter returning html and not the BasicDiff model? The basic diff api should retrurn this BasicDiff model, the html rendering could have been done in the frontend, what was the advantage to render this to html in the backend?

var (
// tplBlock is the whole thing
tplBlock = `{{ define "block" -}}

This comment has been minimized.

@torkelo

torkelo Jun 5, 2017

Member

Same as the question above, interested to know why this html rendering is done in the backend, no other api is returning html

@torkelo

torkelo Jun 5, 2017

Member

Same as the question above, interested to know why this html rendering is done in the backend, no other api is returning html

This comment has been minimized.

@bentranter

bentranter Jun 7, 2017

Contributor

The main reason was calculating the line numbers. We had first tried to keep the JSON diff and basic diff formats separate, and render it on the frontend, but because of Go's random map iteration order, comparing the delta to the JSON represented as an interface{} didn't work since the position of the changes there wouldn't line up with how the JSON turned up on the frontend, so I had tried to come up with a better model on the backend to represent the changes for both formats. I ended up implementing this on the backend just as a proof of concept for the format, intending to render it on the frontend, but since it had worked as-is we just went with it.

Also, while I didn't do the frontend Angular dev work myself, I remember the person who did saying that they had attempted to move the JSON diff rendering to the frontend as just have it consume the tokens emitted by that BasicDiff model, but for large diffs was noticing that Angular was having a hard time rendering the diff efficiently because of the size of the changeset (internally we had changes that would cover around 3000 lines), so I think that discouraged us from continuing to move the rendering to the frontend.

@bentranter

bentranter Jun 7, 2017

Contributor

The main reason was calculating the line numbers. We had first tried to keep the JSON diff and basic diff formats separate, and render it on the frontend, but because of Go's random map iteration order, comparing the delta to the JSON represented as an interface{} didn't work since the position of the changes there wouldn't line up with how the JSON turned up on the frontend, so I had tried to come up with a better model on the backend to represent the changes for both formats. I ended up implementing this on the backend just as a proof of concept for the format, intending to render it on the frontend, but since it had worked as-is we just went with it.

Also, while I didn't do the frontend Angular dev work myself, I remember the person who did saying that they had attempted to move the JSON diff rendering to the frontend as just have it consume the tokens emitted by that BasicDiff model, but for large diffs was noticing that Angular was having a hard time rendering the diff efficiently because of the size of the changeset (internally we had changes that would cover around 3000 lines), so I think that discouraged us from continuing to move the rendering to the frontend.

@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Jun 5, 2017

Member

Been making good progress on refactoring this and making it mergeable.
https://github.com/grafana/grafana/commits/walmartlabs-master

  1. Refactored some UI to be more in line with where we want to go coding wise (see save modal and save as modal)
  2. Removed restore command, this command duplicated a lot of code that had already been written, for example, get dashboard & get dashboard version queries and save dashboard command. Now the restore operation is composed of the existing queries to read dashboards, dashboard version & save dashboard. This change also fixed the big issue with alerting rules not beeing affected by the restore operation (it is now). https://github.com/grafana/grafana/blob/walmartlabs-master/pkg/api/dashboard.go#L426
  3. Moved the compare code out of sql store as this also could be rewritten to reuse the existing query to read dashboard version. https://github.com/grafana/grafana/blob/walmartlabs-master/pkg/components/dashdiffs/compare.go#L44
  4. Fixed sql migration where dashboard.version was given a version + 1 in version table, and removed getMaxVersion logic. incrementing version should always result in max version. Maybe there was a reason for the getMaxVersion logic that I have missed, in that case we might need it back again :)
  5. Removed some unnecessary error handling in the frontend that caused duplicated alert messages (backendSrv shows backend error messages already if you really want to handle show error yourself you can override the default error handling, this is done the by the metrics panel for example for metric requests).
  6. After restore is completed instead of calling setupDashboard I reload the route, this is much safer and easier way to make sure the dashboard is in a correct state after a restore.
  7. Added orgId to some of the queries & to the restore command, still missing from the compare operation.

Don't be discouraged by all these changes, this is still an amazing PR with terrific quality, just needed to make a few aesthetic changes and to improve maintainability & reduce code duplication.

Hope to merge this tomorrow after more refactoring and testing.

Next step for this feature is documentation. Think we can handle that in a new issue/PR :)

Member

torkelo commented Jun 5, 2017

Been making good progress on refactoring this and making it mergeable.
https://github.com/grafana/grafana/commits/walmartlabs-master

  1. Refactored some UI to be more in line with where we want to go coding wise (see save modal and save as modal)
  2. Removed restore command, this command duplicated a lot of code that had already been written, for example, get dashboard & get dashboard version queries and save dashboard command. Now the restore operation is composed of the existing queries to read dashboards, dashboard version & save dashboard. This change also fixed the big issue with alerting rules not beeing affected by the restore operation (it is now). https://github.com/grafana/grafana/blob/walmartlabs-master/pkg/api/dashboard.go#L426
  3. Moved the compare code out of sql store as this also could be rewritten to reuse the existing query to read dashboard version. https://github.com/grafana/grafana/blob/walmartlabs-master/pkg/components/dashdiffs/compare.go#L44
  4. Fixed sql migration where dashboard.version was given a version + 1 in version table, and removed getMaxVersion logic. incrementing version should always result in max version. Maybe there was a reason for the getMaxVersion logic that I have missed, in that case we might need it back again :)
  5. Removed some unnecessary error handling in the frontend that caused duplicated alert messages (backendSrv shows backend error messages already if you really want to handle show error yourself you can override the default error handling, this is done the by the metrics panel for example for metric requests).
  6. After restore is completed instead of calling setupDashboard I reload the route, this is much safer and easier way to make sure the dashboard is in a correct state after a restore.
  7. Added orgId to some of the queries & to the restore command, still missing from the compare operation.

Don't be discouraged by all these changes, this is still an amazing PR with terrific quality, just needed to make a few aesthetic changes and to improve maintainability & reduce code duplication.

Hope to merge this tomorrow after more refactoring and testing.

Next step for this feature is documentation. Think we can handle that in a new issue/PR :)

@@ -223,6 +223,14 @@ func (hs *HttpServer) registerRoutes() {
// Dashboard
r.Group("/dashboards", func() {
r.Combo("/db/:slug").Get(GetDashboard).Delete(DeleteDashboard)
r.Get("/db/:dashboardId/versions", wrap(GetDashboardVersions))

This comment has been minimized.

@torkelo

torkelo Jun 6, 2017

Member

had to change this to /id/:dashboardId/ not super elegant but it's better than having a slug and and id in the same URL segment,

@torkelo

torkelo Jun 6, 2017

Member

had to change this to /id/:dashboardId/ not super elegant but it's better than having a slug and and id in the same URL segment,

width: 110%;
}
input + label::before, input + label::after {

This comment has been minimized.

@torkelo

torkelo Jun 6, 2017

Member

all these overrides classes is not a good & maintainable way to write css. If you need overrides make them modifiers to the original class, never override styles in a different component like this, it's the path to spaghetti css :)

By class modifiers, I mean something like this:https://github.com/grafana/grafana/blob/master/public/sass/components/_gf-form.scss#L12

Overrrides other styles like what is done all over this is to be avoided, will try to clean it up

@torkelo

torkelo Jun 6, 2017

Member

all these overrides classes is not a good & maintainable way to write css. If you need overrides make them modifiers to the original class, never override styles in a different component like this, it's the path to spaghetti css :)

By class modifiers, I mean something like this:https://github.com/grafana/grafana/blob/master/public/sass/components/_gf-form.scss#L12

Overrrides other styles like what is done all over this is to be avoided, will try to clean it up

This comment has been minimized.

@torkelo

torkelo Jun 6, 2017

Member

was able to get rid of all these overrides and align the styles more with standard filter-table styles :) (while keeping the look 95% the same)
88da3a9

@torkelo

torkelo Jun 6, 2017

Member

was able to get rid of all these overrides and align the styles more with standard filter-table styles :) (while keeping the look 95% the same)
88da3a9

This comment has been minimized.

@bentranter

bentranter Jun 7, 2017

Contributor

Thanks again for the refactoring and the feedback, and I'll make sure I don't write CSS like that ever again :)

@bentranter

bentranter Jun 7, 2017

Contributor

Thanks again for the refactoring and the feedback, and I'll make sure I don't write CSS like that ever again :)

date = moment.isMoment(date) ? date : moment(date);
const format = 'YYYY-MM-DD HH:mm:ss';
return this.dashboard.timezone === 'browser' ?

This comment has been minimized.

@torkelo

torkelo Jun 7, 2017

Member

Just for future reference, there is a formatDate function on the dashboard model that does this, so checking timezone and having date formats all over the code base is not needed. All date formatting is done via this function:

https://github.com/grafana/grafana/blob/master/public/app/features/dashboard/model.ts#L247

@torkelo

torkelo Jun 7, 2017

Member

Just for future reference, there is a formatDate function on the dashboard model that does this, so checking timezone and having date formats all over the code base is not needed. All date formatting is done via this function:

https://github.com/grafana/grafana/blob/master/public/app/features/dashboard/model.ts#L247

<section ng-if="ctrl.diff === 'basic'">
<p class="small muted">
<strong>Version {{new}}</strong> updated by
<span>{{ctrl.getMeta(new, 'createdBy')}} </span>

This comment has been minimized.

@torkelo

torkelo Jun 7, 2017

Member

for future reference, never use functions in angular bindings, bad performance & bad practice. Create view models instead.

@torkelo

torkelo Jun 7, 2017

Member

for future reference, never use functions in angular bindings, bad performance & bad practice. Create view models instead.

<em>Fetching changes&hellip;</em>
</div>
<div ng-if="!ctrl.loading" ng-init="new = ctrl.selected[0]; original = ctrl.selected[1]">

This comment has been minimized.

@torkelo

torkelo Jun 7, 2017

Member

trying to stay away from using ng-init and creating variables in the "scope" directly from the view, creates messy templates & logic that can be hard to maintain.

@torkelo

torkelo Jun 7, 2017

Member

trying to stay away from using ng-init and creating variables in the "scope" directly from the view, creates messy templates & logic that can be hard to maintain.

@torkelo torkelo merged commit 77c046a into grafana:master Jun 7, 2017

1 of 2 checks passed

license/cla Contributor License Agreement is not signed yet.
Details
ci/circleci Your tests passed on CircleCI!
Details
@torkelo

This comment has been minimized.

Show comment
Hide comment
@torkelo

torkelo Jun 7, 2017

Member

Just completed my refactorings. There were some big final changes, will try to explain those.

  1. Change the compare api from GET /api/dashboards/db/:dashboardId/compare/A...B/ I wanted to make it possible in the future to add the ability to compare against the current unsaved changes. The Unsaved Changes modal can show up unexpected sometimes, being able to see what you changed before saving would be very helpful.

So I changed the api from a GET to a POST (so it can also send the current dashboard model).
Example:

POST  /api/dashboards/calculate-diff

{
  "new": {
    "dashboardId": 59,
    "version": 6
  },
  "base": {
    "dashboardId": 59,
    "version": 5
  },
  "diffType": "basic"
}

Then instead of sending in dashboardId and version the frontend can send the unsaved dashboard model:

Example:

{
  "new": {
    "unsavedDashboard": {
          "...": "..."
     }
  },
  "base": {
    "dashboardId": 59,
    "version": 5
  },
  "diffType": "basic"
}
  1. Added orgId to dashboard version queries (so you cannot do version lookups or dashboard compares against dashboards your are not allowed to access)

  2. Updated and simplified the HistoryListCtrl code. Changed the diff type name from html to json, in the backend this diff mode was referred to as DiffJSON (in the DiffType), but in the frontend it was named html which was a bit confusing.

  3. One change that I haven't explained is the change from Changelog to Version history. Changelog was an interesting name but did not feel perfectly natural. For example, When you save a dashboard you create & update something. Saving to changelog felt like the wrong term, Changelog usually just contains notes about changes not the actual changes. You can't restore data from a changelog (as the term is usually used), only gather info on what has changed.

  4. Another change that I haven't explained is that I removed the ability to directly save & specify save message from the unsaved changes dialog. This made this dialog a lot busier and harder to at a quick glance understand. In my experience you mostly discard the changes when this appear as you frequently open dashboards and make temporary changes. So having to click Save and the get the normal save modal is ok I feel as it makes the unsaved changes modal easier to parse quickly.

Member

torkelo commented Jun 7, 2017

Just completed my refactorings. There were some big final changes, will try to explain those.

  1. Change the compare api from GET /api/dashboards/db/:dashboardId/compare/A...B/ I wanted to make it possible in the future to add the ability to compare against the current unsaved changes. The Unsaved Changes modal can show up unexpected sometimes, being able to see what you changed before saving would be very helpful.

So I changed the api from a GET to a POST (so it can also send the current dashboard model).
Example:

POST  /api/dashboards/calculate-diff

{
  "new": {
    "dashboardId": 59,
    "version": 6
  },
  "base": {
    "dashboardId": 59,
    "version": 5
  },
  "diffType": "basic"
}

Then instead of sending in dashboardId and version the frontend can send the unsaved dashboard model:

Example:

{
  "new": {
    "unsavedDashboard": {
          "...": "..."
     }
  },
  "base": {
    "dashboardId": 59,
    "version": 5
  },
  "diffType": "basic"
}
  1. Added orgId to dashboard version queries (so you cannot do version lookups or dashboard compares against dashboards your are not allowed to access)

  2. Updated and simplified the HistoryListCtrl code. Changed the diff type name from html to json, in the backend this diff mode was referred to as DiffJSON (in the DiffType), but in the frontend it was named html which was a bit confusing.

  3. One change that I haven't explained is the change from Changelog to Version history. Changelog was an interesting name but did not feel perfectly natural. For example, When you save a dashboard you create & update something. Saving to changelog felt like the wrong term, Changelog usually just contains notes about changes not the actual changes. You can't restore data from a changelog (as the term is usually used), only gather info on what has changed.

  4. Another change that I haven't explained is that I removed the ability to directly save & specify save message from the unsaved changes dialog. This made this dialog a lot busier and harder to at a quick glance understand. In my experience you mostly discard the changes when this appear as you frequently open dashboards and make temporary changes. So having to click Save and the get the normal save modal is ok I feel as it makes the unsaved changes modal easier to parse quickly.

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