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

[DPE-4683] socket config flag #343

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented Aug 23, 2024

  • Add configuration flag to use socket
  • Pass socket endpoint and URI based on the config flag

Copy link

codecov bot commented Aug 23, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 75.05%. Comparing base (cb02311) to head (6d77cf2).

Files with missing lines Patch % Lines
src/relations/pgbouncer_provider.py 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #343      +/-   ##
==========================================
- Coverage   75.20%   75.05%   -0.16%     
==========================================
  Files           9        9              
  Lines        1319     1323       +4     
  Branches      239      240       +1     
==========================================
+ Hits          992      993       +1     
- Misses        254      256       +2     
- Partials       73       74       +1     

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

config.yaml Outdated
Comment on lines 24 to 28
local_connection_type:
default: localhost
description: |
Type of local connectivity to provide. Must be either 'localhost' or 'socket'. Only applies to the database interface.
type: string
Copy link
Contributor Author

Choose a reason for hiding this comment

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

vIP value will already control how exposed connections will behave, so adding config just for local connections.

Base automatically changed from dpe-4683-socket to main August 27, 2024 12:08
@dragomirp dragomirp force-pushed the dpe-4683-socket-config-flag branch from 4b1684c to 3bcf09c Compare August 27, 2024 12:11
@dragomirp dragomirp requested a review from 7annaba3l September 5, 2024 14:50
@7annaba3l
Copy link

Let's rather have as possible values:
"tcp" instead of "socket"
"uds" instead of "local"

@dragomirp dragomirp marked this pull request as ready for review September 10, 2024 09:45
@dragomirp dragomirp requested a review from delgod September 10, 2024 09:45
Copy link
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

Please sign naming with Mohamed. LGTM right after.

Comment on lines +24 to +29
local_connection_type:
default: tcp
description: |
Type of local connectivity to provide. Must be either 'tcp' or 'uds'. Only applies to the database interface.
type: string

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you sign this with Mohamed/Mykola/Jon (spec...). I am afraid it will be rejected.

P.S. I would use tcp or socket (instead of uds). Sure, 'the color of the button' story.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, just noticed #343 (comment)
@7annaba3l please sign the option name and values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants