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

Refactor forwarder to use updated Options pattern from sdk-ovs #363

Conversation

bszirtes
Copy link
Contributor

Description

Refactoring forwarder-ovs to use the updated Options patterns from sdk-ovs, just as it has been implemented in the forwarder-vpp.

Depends on: networkservicemesh/sdk-ovs#320

Issue link

networkservicemesh/sdk-ovs#319

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionality
  • Documentation
  • Refactoring
  • CI

Copy link
Contributor

@ljkiraly ljkiraly left a comment

Choose a reason for hiding this comment

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

Seems good to me

@denis-tingaikin
Copy link
Member

@bszirtes It seems like CI is stuck. Could you rebase the branch to the latest main and re-push changes?

@bszirtes bszirtes force-pushed the refactor-chains-options-pattern branch from 6e1c5ec to d2f950d Compare April 22, 2024 11:08
@bszirtes
Copy link
Contributor Author

@denis-tingaikin I just tried rebasing, and it indicated that everything is up to date. Even though, I did a force-push after the rebasing, but it appears that nothing has changed. Do you have any other suggestions?

@denis-tingaikin
Copy link
Member

@bszirtes Looks weird. There might be something on the GitHub actions side. I just pushed a test branch #368, and CI has started. Could you try creating a new branch based on this and trying to open it?

@bszirtes
Copy link
Contributor Author

@denis-tingaikin Could you please elaborate a bit more on what I should try to open? Should I create a new branch and open a new PR with the same code?

@denis-tingaikin
Copy link
Member

@bszirtes Yes, you may simply use git switch -c to create a new branch based on these changes and open a new PR that I expect should fix the CI. 

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Apr 22, 2024

@bszirtes One second. It seems like your branch just has conflicts that must be resolved.

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Apr 22, 2024

Could you try this?

git switch refactor-chains-options-pattern
git fetch origin
git rebase -X theirs origin/main
git push $forkName refactor-chains-options-pattern -f

@bszirtes bszirtes force-pushed the refactor-chains-options-pattern branch from d2f950d to 885de78 Compare April 22, 2024 12:15
@bszirtes
Copy link
Contributor Author

@denis-tingaikin I just tried.

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Apr 22, 2024

OK, something changed, and it seems like we're headed in the right direction 😀

This should repair the branch.

git branch -m  refactor-chains-options-pattern-tmp
git checkout origin/main
git switch -c refactor-chains-options-pattern
git cherry-pick 885de7831142d881296967b75a74361d1299fb41
git push $forkName refactor-chains-options-pattern -f

@bszirtes
Copy link
Contributor Author

@denis-tingaikin I got the following after I executed git cherry-pick 885de7831142d881296967b75a74361d1299fb41 :

Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging main.go
CONFLICT (content): Merge conflict in main.go
error: could not apply 885de78... Refactor forwarder to use updated Options pattern from sdk-ovs
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

Should I try to resolve these manually?

@denis-tingaikin
Copy link
Member

Yes, it can be fixed manually; as an alternative, you may do this.

git cherry-pick --abort
git cherry-pick -X theirs 885de7831142d881296967b75a74361d1299fb41

@bszirtes bszirtes force-pushed the refactor-chains-options-pattern branch from 885de78 to e97725d Compare April 22, 2024 12:33
@bszirtes
Copy link
Contributor Author

bszirtes commented Apr 22, 2024

@denis-tingaikin I just pushed again, but it seems to me that I have even more conflicts somehow. Should I create a new PR with the same changes?

@denis-tingaikin
Copy link
Member

denis-tingaikin commented Apr 22, 2024

It looks better, but the branch is still has conflicts

image

OK, I see. You're using a fork that has a 3-year diff; that's why we face problems.

This should fix the problem with the diff

git remote add nsm-origin https://github.com/networkservicemesh/cmd-forwarder-ovs.git
git fetch nsm-origin
git branch -m  refactor-chains-options-pattern-tmp
git checkout nsm-origin/main
git switch -c refactor-chains-options-pattern
git cherry-pick -X theirs 885de7831142d881296967b75a74361d1299fb41
git push $forkName refactor-chains-options-pattern -f

Depends on: networkservicemesh/sdk-ovs#320

Signed-off-by: Botond Szirtes <botond.szirtes@est.tech>
@bszirtes bszirtes force-pushed the refactor-chains-options-pattern branch from e97725d to 659e841 Compare April 22, 2024 12:45
@bszirtes
Copy link
Contributor Author

@denis-tingaikin Well, it looks like it passed the checks. Thank you for the assistance!

Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

Woot! Problems are fixed 💪

LGTM.

@denis-tingaikin denis-tingaikin merged commit 0d838b5 into networkservicemesh:main Apr 22, 2024
15 checks passed
nsmbot pushed a commit to networkservicemesh/deployments-k8s that referenced this pull request Apr 22, 2024
…d-forwarder-ovs@main

PR link: networkservicemesh/cmd-forwarder-ovs#363

Commit: 0d838b5
Author: Denis Tingaikin
Date: 2024-04-22 15:50:52 +0300
Message:
  - Merge pull request #363 from Nordix/refactor-chains-options-pattern
Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot added a commit to networkservicemesh/deployments-k8s that referenced this pull request Apr 22, 2024
…d-forwarder-ovs@main (#11895)

PR link: networkservicemesh/cmd-forwarder-ovs#363

Commit: 0d838b5
Author: Denis Tingaikin
Date: 2024-04-22 15:50:52 +0300
Message:
  - Merge pull request #363 from Nordix/refactor-chains-options-pattern

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
Co-authored-by: NSMBot <nsmbot@networkservicmesh.io>
@bszirtes bszirtes deleted the refactor-chains-options-pattern branch April 23, 2024 08:51
This pull request was closed.
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.

3 participants