-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix(): Reconciles port numbers in the ovpn client deployment #333
Conversation
one issue I notices the commits are unverified (No user is associated with the committer email). We require every contributor to certify that they are legally permitted to contribute to our project. A contributor expresses this by consciously signing their commits. please follow this link. Let us know if you need any help. |
4e8de96
to
8998f6e
Compare
@narmidm the commits are verified now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from some small nits It's Looks great! Thanks for adding this and also for fixing the flaky test 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @kon3m
Thanks for reviewing @Rahul-D78 |
@kon3m Please mark the PR as ready for review when you are done. I will review it as soon as possible. |
@narmidm i have found an issue with this fix where in some cases distinct ports do not get assigned to the client deployments or to put in another way both the clients are connected to the same nodePort which should not happen. Thats the reason i have converted the PR to draft. Now i have updated the PR adding the fix for that as well, could you please review it? |
@kon3m , can you rebase with the latest master. it has some fixes of E2E pipeline. After That I will trigger the E2E again. |
3984ab8
to
ceddb76
Compare
@narmidm done, i have just rebased with master and pushed. |
237dc66
to
d72db75
Compare
Urgent CRD changes are slated for release, and we will wait a couple of days for this PR to merge. @kon3m, you will likely need to sync your branch as well. |
* feat(): api changes for no-net slice Signed-off-by: Mridul Gain <[email protected]> * feat(): block reconcilation of networking part Signed-off-by: Mridul Gain <[email protected]> * cluster reconciler changes for no net Signed-off-by: Mridul Gain <[email protected]> * skip workerslice health for no-net deployment Signed-off-by: Mridul Gain <[email protected]> * update slice reconciler for no-net Signed-off-by: Mridul Gain <[email protected]> * fix(): update cluster status to show network present or not Signed-off-by: Mridul Gain <[email protected]> * fix(): ITs & go version update Signed-off-by: Mridul Gain <[email protected]> * fix(): update apis tag Signed-off-by: Mridul Gain <[email protected]> --------- Signed-off-by: Mridul Gain <[email protected]>
Signed-off-by: kon3m <[email protected]>
Signed-off-by: kon3m <[email protected]>
Signed-off-by: kon3m <[email protected]>
Signed-off-by: kon3m <[email protected]>
Signed-off-by: kon3m <[email protected]>
Signed-off-by: kon3m <[email protected]>
3983917
to
5d7a82a
Compare
Signed-off-by: kon3m <[email protected]>
5d7a82a
to
c613169
Compare
@narmidm @Rahul-D78 @bharath-avesha Closing this PR and will open a new/clean one as multiple authors and files got added which are not related to this PR. Will comment the new PR once i open it. This is the newly opened PR : 368 |
Opened a new clean PR |
Description
Fixes #309
How Has This Been Tested?
Tested it manually using two single node kind clusters, will write test cases soon using envtest and update the PR
Checklist:
go fmt
Does this PR introduce a breaking change?