add data source influxdb #88
Conversation
KiaraGrouwstra
changed the title
There was a problem hiding this comment.
Exciting to see this addition! The overall approach here LGTM. I left some initial comments.. I realize this is still at a draft phase. To get this to shipping, it'll also need unit tests. You can have a look at some of the other database hooks for inspiration. Also, this: https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#automated-tests
Let me know if you have any questions. And thank you!
| ''' | ||
| Return whether dump streaming is used for this hook. (Spoiler: It isn't.) | ||
| ''' | ||
| return False |
There was a problem hiding this comment.
This should be True, because you're actually using streaming (returning processes) below!
| ''' | ||
| Given a base directory, make the corresponding dump path. | ||
| ''' | ||
| return dump.make_data_source_dump_path(base_directory, 'infludb_databases') |
There was a problem hiding this comment.
Missing the "x" in "influxdb_databases". 😄
| if dry_run: | ||
| continue |
There was a problem hiding this comment.
You could move this to after build_dump_command() so that a dry run simulates a little bit more before having to bail. Do not feel strongly.
| if dump_format == 'directory': | ||
| dump.create_parent_directory_for_dump(dump_filename) | ||
| execute_command(command, shell=True) |
There was a problem hiding this comment.
If I'm reading this correctly, this code path will never get hit, because there isn't a dump format in the schema.
| database.get('hostname'), | ||
| database.get('port'), | ||
| ) | ||
| dump_format = database.get('format', 'archive') |
There was a problem hiding this comment.
No dump format is present in the schema...?
| token = connection_params['token'] or database.get('token') | ||
| return ( | ||
| ('influx-cli', 'backup',) | ||
| + (('--skip-verify',) if skip_verify else ()) |
There was a problem hiding this comment.
This variable doesn't look like it's set...? Same for several below.
| dump.make_data_source_dump_filename( | ||
| make_dump_path(borgmatic_source_directory), name, hostname='*' | ||
| ), |
There was a problem hiding this comment.
You can probably safely remove this one. Use of the borgmatic "source directory" predates this InfluxDB hook, so there should be no old borgmatic archives with InfluxDB dumps in this source directory location.
| command = ['influx-cli', 'restore'] | ||
| if hostname: | ||
| command.extend(('--host', hostname)) |
There was a problem hiding this comment.
You could construct this command declaratively instead of imperatively like you did in build_dump_command().
Do not feel strongly.
| http_debug = database.get('http_debug') | ||
| full = database.get('full') | ||
|
|
||
| command = ['influx-cli', 'restore'] |
There was a problem hiding this comment.
Similar to dumping, you could consume the same config option here to allow the user to optionally override the influx-cli path. See other database hooks for examples.
| def dump_data_sources( | ||
| databases, | ||
| config, | ||
| log_prefix, |
There was a problem hiding this comment.
Heads up that I have a branch where log prefix will be going away. Nothing for you to do about it now though.
https://projects.torsion.org/borgmatic-collective/borgmatic/pulls/980
| type: string | ||
| description: | | ||
| HTTP address of InfluxDB. | ||
| default: http://127.0.0.1:8086 |
There was a problem hiding this comment.
FYI I don't think these defaults are currently consumed by borgmatic, so they might not make it into the borgmatic config generate output. It'd be good to test that.
see https://projects.torsion.org/borgmatic-collective/borgmatic/issues/450