-
Notifications
You must be signed in to change notification settings - Fork 267
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
modify syncd init script for supporting yml #1411
base: master
Are you sure you want to change the base?
Conversation
if [ ! -z "$line" ];then | ||
if [ "${line::1}" == '#' ];then | ||
echo $line >> $to_file | ||
elif [ "$line" == "[Overwrite Section]" ];then |
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.
If there is an extra space at end, no match will happen, right? How to consider this scenario?
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.
Yes, the script will skip empty lines
echo "Merge properties with override $override" | ||
else | ||
sedline=${line%=*} | ||
if grep -q $sedline $to_file ;then |
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.
@geans-pin If there is "a=1" in overwrite section and "aa=1" in to_file, the sedline is a, but grep -q $sedline $to_file, still return true for this case, right? Will aa=1 be overwritten by a=1?
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.
In the config property, we grep for the property name. It's unlikely to have the a and aa case.
The property name will be coming along with a meaningful naming prefix.
if [ ! -z "$PLT_CONFIG_YML" ] && [ -f $PLATFORM_DIR/common_config_support ]; then | ||
cp -f $HWSKU_DIR/*.config.yml /tmp | ||
cp -f /etc/sai.d/sai.profile /tmp | ||
CONFIG_YML=$(find /tmp -name '*.yml') |
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.
@geans-pin what about this config.bcm file? It doesn't have .yml suffix, but it is yaml format, how to deal with it? https://github.com/sonic-net/sonic-buildimage/blob/b73d613bf581076192dd0150cb35d6d2de6645b1/device/arista/x86_64-arista_7060dx5_64s/Arista-7060DX5-64S/th4-a7060dx5-64s.config.bcm#L4
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.
We use the file name .yml suffix to identify the file format. Can you change the sai.profile and *.config.bcm to *.yml as following BRCM SDK rule ?
if [ ! -z "$line" ];then | ||
if [ "${line::1}" == '#' ];then | ||
echo " $line" >> $to_file | ||
elif [ "$line" == "[Overwrite Section]"];then |
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.
@geans-pin Overwrite section should be after [Normal Section], if we move overwrite section before [Normal Section], the logic here will not work and will overwrite all properties, is it right? Do we have a comment to remind this?
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.
Yes, good catch. Let's have a comment in the script and HLD
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.
Had added the comment in HLD for reminding this
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.
Also add comment in script
what is motivation here? please provide extended descrption for this pr |
I had added the comment to remind this in HLD. Please check the PR of HLD |
still not description in PR and seems like build is failing |
Also, in the HLD PR. Please check the following description. #Note, the Overwrite Section should be located after normal section in the common config file, otherwise the logic will overwrite all properties |
here: #1411 (comment) no description |
No description provided.