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.

Sign up for GitHub

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

Jump to bottom

[installer] Fix variable inside machine.conf caused install.sh error #6600

Merged
merged 4 commits into from
Feb 4, 2021
Merged

[installer] Fix variable inside machine.conf caused install.sh error #6600

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
538dc42
[installer] Fix onie_discovery variable caused install.sh error
kuanyu99 Jan 20, 2021
b085c74
[installer] Fix minor bug of install.sh
kuanyu99 Feb 1, 2021
72514f9
[installer] Handle different type of machine.conf
kuanyu99 Feb 2, 2021
ec36f85
[installer] Add a unit test for read_conf_file function
kuanyu99 Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions installer/x86_64/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,21 @@ _trap_push() {
}
_trap_push true

read_conf_file() {
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

read_conf_file [](start = 0, length = 14)

Could you write a unit test to cover old cases and your new feature? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. But what kind of the test will be better and suitable?
Add a test inside sonic-mgmt and be independent from building. Or, add additional process to test this during building.
I haven't found similar case of this type of unit test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

read_conf_file() is complex. I expect you have tested it locally. Could you just automate the test in another shell script or a python test runner.
In future we may either improve the code or the testcases.

In reply to: 568342160 [](ancestors = 568342160)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just copied the function to another script and test it with a sample machine.conf.
The test can only be triggered manually for now.

local conf_file=$1
while IFS='=' read -r var value
do
f_char=$(echo $var | cut -c1)
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do you assume the # appear as first char of a line? If yes, can you assume it could appear anywhere on a line? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your suggestion is helpful. I modify this part to remove the string behind #.

[ "$f_char" = "#" ] && continue
[ -z "$var" ] && continue
# remove double quote in the beginning
tmp_val=${value#\"}
# remove double quote in the end
value=${tmp_val%\"}
eval "$var=\"$value\""
Copy link
Collaborator

@qiluo-msft qiluo-msft Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible to use declare instead of eval ? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not familiar with declare. What is the advantage to use declare instead of eval?

Copy link
Collaborator

Choose a reason for hiding this comment

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

declare is safer than eval. I know it in bash, not sure about this environment (sh).

In reply to: 568218112 [](ancestors = 568218112)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it in sh environment. There is no declare command.

done < "$conf_file"
}

# Main
set -e
cd $(dirname $0)
Expand All @@ -37,7 +52,7 @@ else
fi

if [ -r ./machine.conf ]; then
. ./machine.conf
read_conf_file "./machine.conf"
fi

if [ -r ./onie-image.conf ]; then
Expand All @@ -54,9 +69,9 @@ fi

# get running machine from conf file
if [ -r /etc/machine.conf ]; then
. /etc/machine.conf
read_conf_file "/etc/machine.conf"
elif [ -r /host/machine.conf ]; then
. /host/machine.conf
read_conf_file "/host/machine.conf"
elif [ "$install_env" != "build" ]; then
echo "cannot find machine.conf"
exit 1
Expand Down