Fix losing expose ports for connectors on local environments#14177
Conversation
vlad-lyutenko
requested review from
ebyhr,
findepi,
hashhar,
ssheikin and
wendigo
vlad-lyutenko
changed the title
vlad-lyutenko
changed the title
vlad-lyutenko
changed the title
There was a problem hiding this comment.
nit: Extract constant for image name.
There was a problem hiding this comment.
I think this is one of the examples where we should consider using an environment variable to decide whether to bind to fixed or random port.
Multiple booleans side by side + 1 additional constructor (meant to be used only in *QueryRunner class) seems a little more work than required.
There was a problem hiding this comment.
Maybe it could be done as a follow up?
vlad-lyutenko
force-pushed
the
vlad-lyutenko/fixed_port_for_connectors
branch
from
3bc726c to
ec2292d
Compare
vlad-lyutenko
changed the title
There was a problem hiding this comment.
Fix for method exposing port on local environment
This fix prevents loosing ports which were exposed
after we call this method
I couldn't understand what was wrong. I ran PostgreSqlQueryRunner with/without this commit, but the result looks same to me. Could you describe more details?
There was a problem hiding this comment.
@ebyhr
Call to this method add some override rules, which will be executed later.
So it's possible such situation.
- we collect current expose ports by calling this method (but not yet execute it)
- .... add more expose ports in other code ....
- actually apply overrides but only with values from 1) missing values from 2)
With Postgres it works because all exposed ports was added before 1)
There was a problem hiding this comment.
@vlad-lyutenko can you add some of this detailed context to the commit message?
This fix prevents loosing ports, which could be added as exposed, between call this method and actual execution of applying override rules
vlad-lyutenko
force-pushed
the
vlad-lyutenko/fixed_port_for_connectors
branch
from
ec2292d to
a225c60
Compare
5 participants
Description
This PR contains fix which prevents lose exposed ports, for functionality introduced in this PR:
#14037
Non-technical explanation
Release notes
(x) This is not user-visible and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: