Implement simple InfluxDB client

[wcoenraads]

Adds a simple InfluxDB client as described in #6. It's important to note that the current implementation does not escape the parameters to the query, so it should not be used with untrusted input.

[codecov]

Codecov Report

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

Project coverage is 52.77%. Comparing base (bf5df78) to head (cf460b0).
Report is 22 commits behind head on main.

❗ Current head cf460b0 differs from pull request most recent head 521ca0b. Consider uploading reports for the commit 521ca0b to get more accurate results

Files	Patch %	Lines
src/influx.jl	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #19      +/-   ##
==========================================
- Coverage   56.54%   52.77%   -3.77%     
==========================================
  Files           5        6       +1     
  Lines         168      180      +12     
==========================================
  Hits           95       95              
- Misses         73       85      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[suvayu]

A few comments:

• Could you wrap the client in a module so that everything InfluxDB is in its own namespace?

  module InfluxDB
  # current contents of influx.jl
  end # module InfluxDB

• With the above change, maybe it is best to rename InfluxClient to just Client
• Do you have any idea how to write tests for this module?

[suvayu]

I don't think this kind of convenience constructors can be inner. You should move this to outer, then you cannot use new. So, probably this:

Suggested change

	InfluxClient(
	host::String,
	database::String,
	port::Int = 8086,
	path::String = "query",
	username::String = "",
	password::String = "",
	) = new(host, database, port, username, password, path)
	end
	end
	InfluxClient(
	host::String,
	database::String,
	) = InfluxClient(host, database, 8086, "query", "", "")

[clizbe]

@wcoenraads Is this waiting on Suvayu's return?

[suvayu]

@clizbe if my review comments are addressed, we can merge this as a first implementation. I'm more concerned about the lack/difficulty of testing. We need to solve it, but not necessarily in this PR.

[wcoenraads]

These comments/notifications completely passed me by - apologies! I'll address your comments and update the PR this afternoon.

As for testing, I'm also unsure. Downstream tests relying on these APIs could use mock data, but the influx client should really be tested against the InfluxDB API as that's the thing it interacts with. We could create a setup with a development DB to test against, but that feels like a lot of effort for little gain at the moment.

[wcoenraads]

• Could you wrap the client in a module so that everything InfluxDB is in its own namespace?
• [...]
• With the above change, maybe it is best to rename InfluxClient to just Client

Good point on the module!

I've renamed InfluxClient to InfluxDBClient to stay consistent with the official API. I'm also not super familiar with the conventional way to do imports in Julia, but I assume using InfluxDBClient instead of just Client should also help prevent conflicts when importing it.

[suvayu]

Thanks @wcoenraads, everything looks good 👍, except I think it would be more consistent if the file was called influxdb.jl.

Sorry, I should have included this comment earlier. I'll comment separately on testing.

[suvayu]

the influx client should really be tested against the InfluxDB API as that's the thing it interacts with.

Indeed, and a separate dev endpoint would also be too much external dependency. I can think of two approaches:

1. As I understood it, the end point doesn't require authentication, and is available inside the TNO network. If that's the case, maybe we can do a test that is skipped if on an outside network. I'm not sure what would a good check here, maybe simply see if the endpoint is reachable, and skip with a warning if not? This way we can atleast test on a dev machine at the office.
2. The other more involved option would be to add a step in CI that starts an influxdb container with an endpoint and some data.

I would say (1) is good enough for now, we can think about (2) later.

WDYT? How about @clizbe, any opinions?

[suvayu]

PS: I think the nightly test failure is due to an upstream packaging issue, ignore for now.

[wcoenraads]

I think it would be more consistent if the file was called influxdb.jl.

Yep, good point - changed!

I can think of two approaches:
[....]

The first approach could work, but seems limited and fragile: tests that can't be run in CI are much less likely to be adhered to, and they would depend on the specifics of an internal DB we only have limited control over.

The second approach is solid, but indeed more work. It might also be beneficial in tests for later components, where we can do end-to-end tests without mocks in between. But that's a concern for later.

[suvayu]

Thanks! I merged this, we can workout the tests in a separate PR.

[clizbe]

Closes #6