feat: create new Metabase client #3970
Conversation
|
Removed vultr server and associated DNS entries |
zz-hh-aa
force-pushed
the
oz/metabase-client-setup
branch
from
9685a81 to
6264cc2
Compare
There was a problem hiding this comment.
Looking good!
A few minor comments and suggestions here and there 👍
I think we should also test this client with unit tests in isolation - a lot of the functionality here could be hit using axios mocks for responses and requests. If you search vi.mocked(axios, true) in the codebase you'll find some existing examples.
|
Thanks for the detailed review @DafyddLlyr 🌟 I believe I have addressed most of your comments but I have one more outstanding question:
Do you think those unit tests should be added to |
There was a problem hiding this comment.
All changes look great thanks, marked all as resolved 👍
Do you think those unit tests should be added to client.test.ts as part of this PR?
I think it's worth doing it in this PR - this way we have an addition to the code base associated with all its requisite tests which is a nice way of working.
By purely testing the client itself (e.g. how it behaves without secrets, the retry logic, passing of API key, error handling) we'll end up with client-specific functionality covered here, and then the Metabase-specific functionality can be tested a little more cleanly also.
for Metabase client
with error handling
removes unnecessary imports and code, and fixes constants
zz-hh-aa
force-pushed
the
oz/metabase-client-setup
branch
from
7323e37 to
b3947be
Compare
| expect(client.defaults.baseURL).toBe(process.env.METABASE_URL_EXT); | ||
| expect(client.defaults.headers["X-API-Key"]).toBe( | ||
| process.env.METABASE_API_KEY, | ||
| ); | ||
| expect(client.defaults.headers["Content-Type"]).toBe("application/json"); | ||
| expect(client.defaults.timeout).toBe(30_000); |
What does this PR do?
createMetabaseClientvalidateInput()function to check that client input is the expected shape & data types