Skip to content

Conversation

aldy505
Copy link
Collaborator

@aldy505 aldy505 commented Oct 4, 2025

Closes #3953

Copy link

codecov bot commented Oct 4, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.49%. Comparing base (c55b93b) to head (34ccf9c).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3982   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files           3        3           
  Lines         197      197           
=======================================
  Hits          196      196           
  Misses          1        1           

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

Copy link
Collaborator

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest adding this after echo "${_group}Setting up GeoIP integration ..." line.

Also geoip.sh may get called from any other directory or using a systemd service, ... .

It's better to check for _detect-container-engine.sh next to script_dir directory.
With a command like this:

script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd -P)

The plan is to make /path/to/install/geoip.sh and ./geoip.sh source _detect-container-engine.sh and make it work.

@aldy505
Copy link
Collaborator Author

aldy505 commented Oct 4, 2025

I'd suggest adding this after echo "${_group}Setting up GeoIP integration ..." line.

Also geoip.sh may get called from any other directory or using a systemd service, ... .

It's better to check for _detect-container-engine.sh next to script_dir directory.
With a command like this:

script_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")" &>/dev/null && pwd -P)

The plan is to make /path/to/install/geoip.sh and ./geoip.sh source _detect-container-engine.sh and make it work.

@aminvakil I was initially thinking to make a separate bash script rather than to tell users to use geoip.sh directly.

So they would execute "./scripts/update-geoip.sh"

What do you think?

@aminvakil
Copy link
Collaborator

@aminvakil I was initially thinking to make a separate bash script rather than to tell users to use geoip.sh directly.

So they would execute "./scripts/update-geoip.sh"

What do you think?

If users were using geoip.sh to update their IPs, they will to continue to do so for quite some time. I'd suggest fixing this anyway.

@aldy505 aldy505 requested a review from aminvakil October 15, 2025 08:13
Copy link
Collaborator

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

script_dir is where script directory is.

aldy505 and others added 3 commits October 15, 2025 19:46
Co-authored-by: Amin Vakil <[email protected]>
Co-authored-by: Amin Vakil <[email protected]>
Co-authored-by: Amin Vakil <[email protected]>
@aldy505 aldy505 requested a review from aminvakil October 15, 2025 12:47
@aldy505 aldy505 merged commit 60da8f6 into master Oct 15, 2025
21 of 22 checks passed
@aldy505 aldy505 deleted the aldy505/fix/geoip-standalone-script branch October 15, 2025 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

install/geoip.sh wrong docs

3 participants