Feat docker geo #8747
Feat docker geo #8747
Conversation
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
app/init.php
Outdated
| } | ||
|
|
||
| return $record; | ||
| }); |
There was a problem hiding this comment.
Missing injection ['ip', 'geodbClient']
app/init.php
Outdated
| return $client; | ||
| }); | ||
|
|
||
| App::setResource('geoRecordDocker', function ($ip, $geodbClient) { |
There was a problem hiding this comment.
Let's incude types, likely string $ip, Client $geodbClient
| try { | ||
| $record = $geodbClient->fetch("/ips/{$ip}", Client::METHOD_GET); | ||
| $record = $record->json(); | ||
| } catch (Throwable $th) { |
There was a problem hiding this comment.
Do we have logger in geoDB HTTP server? If not, I think we should. Logs like these will get lost, so best if geoDB container directly reports to Sentry
There was a problem hiding this comment.
Yes, geoDB server has logger integrated.
app/init.php
Outdated
| return $record; | ||
| }); | ||
|
|
||
| App::setResource('geoRecordEmbeded', function ($ip, $geodb) { |
There was a problem hiding this comment.
Docker will always be there, right? Can we not have Embeded version anymore?
There was a problem hiding this comment.
We want to fallback in case the docker fails due to any issues like network, connection or other unknown issues.
…t-custom-cf-hostnames
- also update the `update bucket` endpoint
app/init.php
Outdated
| return $request->getIp(); | ||
| }, ['request']); | ||
|
|
||
| App::setResource('geodbClient', function () { |
There was a problem hiding this comment.
This one feels a bit like over engineering. We can just create it every time we take the record. The overhead here non-exist.
app/init.php
Outdated
| @@ -1629,6 +1631,76 @@ function getDevice(string $root, string $connection = ''): Device | |||
| return $register->get('geodb'); | |||
| }, ['register']); | |||
|
|
|||
| App::setResource('ip', function ($request) { | |||
There was a problem hiding this comment.
Over engineering. We can just rely on the request object.
app/init.php
Outdated
| return $client; | ||
| }); | ||
|
|
||
| App::setResource('geoRecordDocker', function (string $ip, Client $geodbClient) { |
There was a problem hiding this comment.
We should never name stuff after Docker, at worst it should be a geodb server. I think we can just stick with geodb and expect to get the record object.
app/init.php
Outdated
| return $record; | ||
| }, ['ip', 'geodbClient']); | ||
|
|
||
| App::setResource('geoRecordEmbeded', function (string $ip, Reader $geodb) { |
There was a problem hiding this comment.
I'm a bit confused between the roles of the different objects. I'm pretty sure everything can be much simpler.
| use Utopia\Fetch\Client as UtopiaClient; | ||
| use Utopia\Fetch\Response; | ||
|
|
||
| class Client extends UtopiaClient |
There was a problem hiding this comment.
Can we use utopia/fetch instead? I think @Meldiron created a new improved version of it.
There was a problem hiding this comment.
I'm extending Utopia Fetch client, to be able to set a base URL for every request
There was a problem hiding this comment.
Isn't this possible in utopia/fetch?? Maybe we should add it? Not a blocker, but do ask @Meldiron about it.
…t-custom-cf-hostnames
Improve compression param checks
upgrade utopia storage
…t-custom-cf-hostnames
…t-custom-cf-hostnames
…t-custom-cf-hostnames
…t-custom-cf-hostnames
…t-custom-cf-hostnames
…t-custom-cf-hostnames
What does this PR do?
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist