Feature: add optional managed connection #52700
Conversation
|
Need to add some tests before it can be merged |
|
@Mottimo here I added the feature which allows for only 1 connection to be used with the hook |
Nataneljpwd
force-pushed
the
feature/add-optional-managed-conn
branch
2 times, most recently
from
58a35ef to
9d9c06b
Compare
Nataneljpwd
force-pushed
the
feature/add-optional-managed-conn
branch
from
c374239 to
0559075
Compare
|
@Lee-W fixed the comments, if you can resolve the one's I fixed, otherwise, reply to them and I will fix them |
Nataneljpwd
force-pushed
the
feature/add-optional-managed-conn
branch
from
0559075 to
d9d95a9
Compare
|
@Lee-W, I have updated it according to everything you said |
|
we'll need to fix the CI as well. Also, I'll be out for the conference for the following 2 weeks and might need others help to look into the PR. |
|
Sure, I will fix the ci, it seems to not find the tests for some reason |
|
@Lee-W I have fixed the issue, it was that the ci required a test file for the exceptions file, I added it to the overlooked tests file list that is in the core tests as per convention on other providers |
Nataneljpwd
force-pushed
the
feature/add-optional-managed-conn
branch
from
d9d95a9 to
9d9b581
Compare
|
Just came back. Will take a look this week. Please resolve the comments if you think you've changed it and no discussion is needed. Thanks! |
|
@Lee-W resolved all comments that do not require a discussion, only 1 left |
Nataneljpwd
force-pushed
the
feature/add-optional-managed-conn
branch
from
01d0b98 to
00386fa
Compare
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
Looks better now. We're closer to merge than the previous iteration!
|
@Lee-W, fixed all comments |
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
No major things need to be changed. most of these are nitpicks, but I feel we probably could improve the test coverage
|
@Lee-W fixed all comments, sorry it took me so long, I am just working in parallel on the scheduler starvation bug, not sure if you heard of it, PR number 53492 (in case you want to check it out). |
|
@Nataneljpwd can you fix the above issues? |
|
@eladkal Sure, will fix it today and push |
|
@Lee-W fixed all issues, including merge conflicts |
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
will need to fix the CI failure
|
@Lee-W it seems to be a faulty run If you could rerun the job it should fix it |
|
Looks like tests passed. cc @Lee-W |
|
yep, let's merge it! |
Co-authored-by: Natanel Rudyuklakir <natanelrudyuklakir@gmail.com>
Co-authored-by: Natanel Rudyuklakir <natanelrudyuklakir@gmail.com>
3 participants
As of now, each action performed with the sftp, requires a new connection, this pr allows for all the actions to be optionally done in 1 connection while you have to manage the connection manually
Related: #52289