Added timezone information to the available fields #33
Conversation
WalkthroughThe recent updates enhance the application's geolocation capabilities by integrating the GeoLite2-City database into the Docker setup, and configuring nginx to utilize this database for time zone information retrieval. A new endpoint and additional response fields have been introduced to provide users with time zone data based on their IP addresses, enriching the application's geospatial functionalities. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
skinny
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- Dockerfile (1 hunks)
- nginx/conf.d/geoip2.conf (1 hunks)
- nginx/conf.d/ipinfo.conf (2 hunks)
Additional Context Used
Additional comments not posted (3)
nginx/conf.d/geoip2.conf (1)
15-18: The addition of the GeoLite2-City.mmdb database configuration for retrieving timezone information is correctly implemented and aligns with the PR's objectives. However, for maintainability, consider externalizing configurable values such as the database path and auto-reload interval into a separate configuration file or environment variables. This approach can simplify future updates or changes to these settings.nginx/conf.d/ipinfo.conf (2)
53-55: The addition of the/timezoneendpoint is correctly implemented and serves the PR's objective of making timezone information accessible. Well done!
64-64: The modification to the/jsonendpoint to include thetimezonefield is correctly implemented and aligns with the PR's objectives. This enhances the endpoint's utility by providing additional valuable information.
| RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz | ||
| RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256 | ||
| RUN sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256 | ||
| RUN sha256sum -c GeoLite2-City.tar.gz.sha256 | ||
| RUN tar xvf GeoLite2-City.tar.gz --strip 1 |
There was a problem hiding this comment.
The steps for handling the GeoLite2-City database are correctly implemented, ensuring secure download, verification, and extraction. To optimize the Dockerfile and reduce the number of layers, consider combining these RUN commands into a single RUN command using && to chain the commands together. This can improve the build performance and reduce the image size.
-RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz
-RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256
-RUN sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256
-RUN sha256sum -c GeoLite2-City.tar.gz.sha256
-RUN tar xvf GeoLite2-City.tar.gz --strip 1
+RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz && \
+ wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256 && \
+ sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256 && \
+ sha256sum -c GeoLite2-City.tar.gz.sha256 && \
+ tar xvf GeoLite2-City.tar.gz --strip 1Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz | |
| RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256 | |
| RUN sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256 | |
| RUN sha256sum -c GeoLite2-City.tar.gz.sha256 | |
| RUN tar xvf GeoLite2-City.tar.gz --strip 1 | |
| RUN wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz" -O GeoLite2-City.tar.gz && \ | |
| wget "${MAXMIND_BASE_URL}edition_id=GeoLite2-City&suffix=tar.gz.sha256" -O GeoLite2-City.tar.gz.sha256 && \ | |
| sed 's/GeoLite2-City_[0-9]*.tar.gz/GeoLite2-City.tar.gz/g' -i GeoLite2-City.tar.gz.sha256 && \ | |
| sha256sum -c GeoLite2-City.tar.gz.sha256 && \ | |
| tar xvf GeoLite2-City.tar.gz --strip 1 |
There was a problem hiding this comment.
Cool, thanks for your contribution, @skinny. Please allow me to do some tests, and then we can merge it!
nginx/conf.d/geoip2.conf
Outdated
| @@ -11,3 +11,9 @@ geoip2 /usr/share/GeoIP/GeoLite2-ASN.mmdb { | |||
| $ip_aso source=$remote_addr autonomous_system_organization; | |||
| $ip_as_build_epoch metadata build_epoch; | |||
| } | |||
|
|
|||
| geoip2 /usr/share/GeoIP/GeoLite2-City.mmdb { | |||
| auto_reload 1d; # Automatically reload the database daily | |||
There was a problem hiding this comment.
We don't need those additional comments just for the city database, so let's maintain consistency here.
nginx/conf.d/geoip2.conf
Outdated
| auto_reload 1d; # Automatically reload the database daily | ||
| $ip_time_zone source=$remote_addr location time_zone; # Retrieve the time zone | ||
| } | ||
|
|
There was a problem hiding this comment.
This additional newline should also been removed.
There was a problem hiding this comment.
We should add some documentation about the timezone feature in the README file, allowing users to understand how to utilize this new functionality.
e.g.
- Timezone Endpoint
timezone: Timezone information based on IP address
$ curl https://ipinfo.tw/timezone
Asia/Taipei
{"ip":"3.115.123.234","country_code":"JP","country_name":"Japan","timezone":"Asia/Tokyo","asn":"16509","as_desc":"Amazon.com, Inc.","user_agent":"curl/7.58.0"}
|
@PeterDaveHello removed the comments, newlines and added a bit of documentation on the timezone field |
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- README.md (3 hunks)
Additional comments not posted (5)
README.md (5)
58-58: The server's response now includes timezone information, which is a significant enhancement. It's crucial to ensure that this new feature is well-documented and that examples are provided to guide users on how to use it effectively.
63-63: The JSON response example is clear and informative, showing the new timezone information. This addition enhances the usability of the service for clients that require timezone data.
78-78: The addition of a/timezoneendpoint is a valuable feature for users specifically interested in timezone information. It's important to ensure that this endpoint is tested thoroughly to maintain the reliability of the service.
101-103: Providing an example of the/timezoneendpoint usage is helpful for users. It might be beneficial to include more examples or scenarios where this endpoint could be particularly useful, enhancing the documentation's comprehensiveness.
55-66: > 📝 NOTEThis review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [134-134]
When instructing users to build their own image with a
docker buildcommand, it's mentioned to use aMAXMIND_LICENSE_KEY. It would be helpful to emphasize the importance of keeping this key secure and not exposing it in publicly accessible places, such as GitHub repositories.
| - `wget -qO- https://ipinfo.tw` | ||
| - `curl https://ipinfo.tw` | ||
|
|
||
| Without any specified URI, the server will return IP address, country, AS, and user agent. | ||
| Without any specified URI, the server will return IP address, country, timezone, AS, and user agent. | ||
|
|
||
| If you prefer to receive a machine-readable result, use path `/json` (without trailing slash), e.g. `https://ipinfo.tw/json`, the result will look like: | ||
|
|
||
| ```json | ||
| {"ip":"3.115.123.234","country_code":"JP","country_name":"Japan","asn":"16509","as_desc":"Amazon.com, Inc.","user_agent":"curl/7.58.0"} | ||
| {"ip":"3.115.123.234","country_code":"JP","country_name":"Japan","timezone":"Asia/Tokyo","asn":"16509","as_desc":"Amazon.com, Inc.","user_agent":"curl/7.58.0"} | ||
| ``` | ||
|
|
||
| #### Endpoints |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]
The description of the demo setup mentions "an reverse proxy" which should be corrected to "a reverse proxy" for grammatical accuracy.
- this demo is behind an reverse proxy with https enabled
+ this demo is behind a reverse proxy with https enabled📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-29]
There's a minor typographical error with "http traffic" which should be capitalized as "HTTP traffic" for consistency with standard terminology.
- http traffic will be redirected to use https
+ HTTP traffic will be redirected to use https📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [49-49]
The phrase "pass the it to the container" seems to contain an extra word. It should be corrected for clarity.
- set up an `X-Real-IP` header and pass the it to the container
+ set up an `X-Real-IP` header and pass it to the container
README.md
Outdated
| @@ -46,7 +46,7 @@ Run the server daemon via docker: | |||
| docker run -d --name ipinfo.tw -p 80:8080 peterdavehello/ipinfo.tw:latest | |||
| ``` | |||
|
|
|||
| If you want to put this container behind reverse proxy, set up an `X-Real-IP` header and pass the it to the container, so that it can use the header as the IP of the client. | |||
| If you want to put this container behind reverse proxy, set up an `X-Real-IP` header and pass it to the container, so that it can use the header as the IP of the client. | |||
There was a problem hiding this comment.
This is not related to the changes in the PR, we should leave it alone.
This reverts commit 8ca2ec0.
There was a problem hiding this comment.
Copilot reviewed 1 out of 4 changed files in this pull request and generated no suggestions.
Files not reviewed (3)
- Dockerfile: Language not supported
- nginx/conf.d/geoip2.conf: Language not supported
- nginx/conf.d/ipinfo.conf: Language not supported
My use case also uses the timezone information (available on ipinfo.io) but was not yet available in this self hosted soltution. This PR downloads the City geo database and uses that to return a "best guess" timezone for the users IP.
Summary by CodeRabbit
/timezoneendpoint for retrieving user time zone information./jsonendpoint to includetimezoneinformation in responses.