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

[Aclorch] hard code table name separator to make it compatible with 201803 branch #518

Closed
wants to merge 1 commit into from
Closed

Conversation

keboliu
Copy link
Collaborator

@keboliu keboliu commented Jun 7, 2018

What I did
Hard code the table name separator with a Macro instead of getting it by calling swss-common functions.

Why I did it
On the 201803 branch, the swss-common submodule have an issue when the table obeject constructed without a table separator, to make the fix #sonic-net/sonic-buildimage#1712 also work on the branch, so make this change.

How I verified it
Run ACL/Everflow/CRM test.

Details if related
If decided to update the swss-common submodule on the branch, then this PR not needed anymore.

Copy link
Collaborator

@zhenggen-xu zhenggen-xu left a comment

Choose a reason for hiding this comment

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

Can you elaborate about this issue "On the 201803 branch, the swss-common submodule have an issue when the table obeject constructed without a table separator"? I think we should fix that problem instead if possible.

@jleveque
Copy link
Contributor

jleveque commented Jun 13, 2018

@zhenggen-xu: I created a non-backwards-compatible change to the way swss-common deals with table name separators. I removed many instances of hard-coding so as to allow for multiple table name separators (since we are currently transitioning from ":" to "|" and both are used). The table name separator is now an attribute of each DB.

My commits were not cherry-picked into the 201803 branch. We are only merging small, crucial changes into the branch as we deem necessary. However, @keboliu added features in the master branch that we wanted to pull into 201803, but it relied on my new table name separator changes. This PR is to modify his changes to work with the old swss-common which is present in the 201803 branch. It will have no bearing on the master branch, which doesn't have this issue.

@zhenggen-xu: Scratch all that. It appears we are now going forward with merging the swss-common changes into the 201803 branch. See here: sonic-net/sonic-buildimage#1791

@zhenggen-xu
Copy link
Collaborator

Thanks, assuming the risk has been assessed, it would always be good to move things forward.

@keboliu
Copy link
Collaborator Author

keboliu commented Jun 14, 2018

@jleveque Thanks for the clarification, this is exactly what I intend to fix in this PR.

@keboliu
Copy link
Collaborator Author

keboliu commented Jun 14, 2018

@zhenggen-xu if the related change in master will be merged to 201803 branch, then this PR will not be needed anymore.

@liatgrozovik
Copy link

this is no more required for 201803 branch. hard coded values were for WA which decided later on we should not use

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022
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.

4 participants