Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow dbconfig options #968

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Conversation

iwpnd
Copy link
Member

@iwpnd iwpnd commented Feb 17, 2024

application_name and default_transaction_read_only to be configurable

Closes: #966

@coveralls
Copy link

coveralls commented Feb 17, 2024

Pull Request Test Coverage Report for Build 746b513e9-PR-968

Details

  • -4 of 31 (87.1%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 46.914%

Changes Missing Coverage Covered Lines Changed/Added Lines %
provider/postgis/postgis.go 27 31 87.1%
Totals Coverage Status
Change from base Build b7ad33baf: 0.06%
Covered Lines: 6590
Relevant Lines: 14047

💛 - Coveralls

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

Looks really good! Only a few minor comments / requests and then we can land this. Thanks for tacking this one.

provider/postgis/postgis.go Outdated Show resolved Hide resolved
provider/postgis/postgis.go Outdated Show resolved Hide resolved
Comment on lines +18 to +28
host := ttools.GetEnvDefault("PGHOST", "localhost")
port, err := strconv.Atoi(ttools.GetEnvDefault("PGPORT", "5432"))
// if port is anything but int, fallback to default
if err != nil {
port = 5432
}
database := ttools.GetEnvDefault("PGDATABASE", "tegola")
user := ttools.GetEnvDefault("PGUSER", "postgres")
password := ttools.GetEnvDefault("PGPASSWORD", "postgres")

cs := fmt.Sprintf("postgres://%v:%v@%v:%v/%v", user, password, host, port, database)
Copy link
Member

Choose a reason for hiding this comment

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

Note to self, I need to update our CI's env vars to have connection strings so you don't need to do this juggle.

provider/postgis/postgis.go Show resolved Hide resolved
@iwpnd
Copy link
Member Author

iwpnd commented Feb 24, 2024

Looks really good! Only a few minor comments / requests and then we can land this. Thanks for tacking this one.

Done. My pleasure 🙏

Copy link
Member

@gdey gdey left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ARolek ARolek left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the additional updates.

application_name and default_transaction_read_only to be configurable

Closes: go-spatial#966

fix: compare default_transaction_read_only with uppercase

chore: use const for runtime param keys, update readme
@ARolek ARolek merged commit e84707c into go-spatial:master Feb 27, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to avoid setting default_transaction_read_only when connecting to PostgreSQL?
4 participants