-
Notifications
You must be signed in to change notification settings - Fork 2
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
Restore data sources #9
Conversation
…atasources to make them easier to match on importing.
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.
I think this PR is good solution. I just have two minor remarks, see comments in the review.
main.go
Outdated
@@ -96,21 +96,27 @@ func main() { | |||
case "backup": | |||
// TODO fix logic accordingly with apply-for | |||
doBackup(serverInstance, applyFor, matchDashboard) | |||
fmt.Println("Backup Complete.") |
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.
Maybe better show these messages only in verbose
mode? For CLI apps I try to stay with Unix ideology: utility should silently do it job and complain to stderr only on errors. So stdout should be used only for side effects (for example display listing) or in verbosity mode.
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.
Hrm... I'm two ways on that. You're right that a command line tool like this needs to play nicely with automation (that's pretty much exactly what it's for.) And all of the built in tools I can think of behave that way. However most of those all deal with local things and I've used a few of more complex tools which give you some indication by default.
That being said if we were to output a message by default we would want to add a --silent flag to make the thing shut up and that seems a little un-neccessary. I'll drop these messages and go back to the silent-by-default behavior when I update the PR for the above.
do-restore.go
Outdated
@@ -23,28 +23,137 @@ import ( | |||
"io/ioutil" | |||
"os" | |||
"strings" | |||
"regexp" | |||
"encoding/json" | |||
"github.com/grafana-tools/sdk" |
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.
Let split external import clauses from internal with a space line? It is not a strict requirement of course just minor style notify. I suppose it makes import statement more readable.
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.
That's why I see so much code with a space in the imports. I was unaware of that convention. That makes sense.
…ate native from third party imports and sorted them.
PR Updated |
Of course now it's not restoring so I'll have to figure out what I broke and re-submit. |
See the fixes in #11. |
Fixes #9, see it for the bug explanation. Commits are included in the changeset: * Updated do-restore to switch on restore type like do-backup. * Beginning of datasource restore. Changed the output file format for datasources to make them easier to match on importing. * Un-did datasource filename change. * Working import of datasources. * Cleand up a lot of garbage. * Added some default output so that we know the command has completed. * Removed the default "blah completed" messages. Added a space to separate native from third party imports and sorted them. * Fixed an issue where the restore wasnt actually hapenning. updated some log messages for clarity. * Started some unit tests with github.com/jarcoal/httpmock. Updated gitignore to allow it to include json files within the testdata directory. * Updated order of imports.
Added a filter so that we don't try and import data sources as dashboards (based entirely on filename)
Working restoration of datasource types. Similar filter based on file name.
Nothing for users yet.
I am not 100% thrilled with the way I've implemented some of the functionality so I'm not really expecting this PR to be accepted as is but looking for input on it.
fixes issue #5