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

[config]: Add file check before load_minigraph command #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stcheng
Copy link
Contributor

@stcheng stcheng commented Aug 21, 2018

This will prevent config load_minigraph fail during the
middle of the command execution.

Signed-off-by: Shu0T1an ChenG shuche@microsoft.com

prsunny
prsunny previously approved these changes Aug 21, 2018
Copy link
Contributor

@prsunny prsunny left a comment

Choose a reason for hiding this comment

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

Looks ok. To me it appears more of a defensive check. I'm just thinking the use-case between load_minigraph vs user could execute a config reload.

@stcheng
Copy link
Contributor Author

stcheng commented Aug 21, 2018

yes. I think the use-case for load_minigraph is that the config_db.json file is not easy to modify while right now the minigraph is the source of the 'original configuration' for the switch.

config/main.py Outdated
@@ -321,7 +321,11 @@ def load_mgmt_config(filename):
expose_value=False, prompt='Reload config from minigraph?')
def load_minigraph():
"""Reconfigure based on minigraph."""
#Stop services before config push
# Check /etc/sonic/minigraph.xml file exists
if not os.path.isfile('/etc/sonic/minigraph.xml'):
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 21, 2018

Choose a reason for hiding this comment

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

/etc/sonic/minigraph.xml [](start = 27, length = 24)

/etc/sonic/minigraph.xml [](start = 27, length = 24)

should not be hardcoded. #Closed

@qiluo-msft
Copy link
Contributor

run_command("config qos reload", display_cmd=True)

Since 'qos' is a local function, why we don't directly call it?


Refers to: config/main.py:344 in ef57bac. [](commit_id = ef57bac77bcdbf0e04b0d2b0295484ee524b2331, deletion_comment = False)

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments.

This will prevent config load_minigraph fail during the
middle of the command execution.

Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>
@@ -321,7 +322,11 @@ def load_mgmt_config(filename):
expose_value=False, prompt='Reload config from minigraph?')
def load_minigraph():
"""Reconfigure based on minigraph."""
#Stop services before config push
# Check /etc/sonic/minigraph.xml file exists
if not os.path.isfile(SONIC_MINIGRAPH_PATH):
Copy link
Contributor

@qiluo-msft qiluo-msft Aug 27, 2018

Choose a reason for hiding this comment

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

if not os.path.isfile(SONIC_MINIGRAPH_PATH): [](start = 4, length = 44)

I suggest test it by command sonic-cfggen -m
The absolute path is internal knowledge of cfggen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree here, sonic-cfggen might even do some sanity check here.

mihirpat1 pushed a commit to mihirpat1/sonic-utilities that referenced this pull request Sep 15, 2023
…nic-net#306)

* update the return for update_firmware api's failure case when the image file doesn't exist

* update comments

* update comment

* update comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants