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

Support golden config in db migrator #3076

Merged
merged 19 commits into from
Dec 22, 2023
Merged

Conversation

ganglyu
Copy link
Contributor

@ganglyu ganglyu commented Dec 12, 2023

What I did

Need to support golden config in db migrator.

How I did it

If there's golden config json, read from golden config instead of minigraph.
And db migrator will use golden config data to generate new configuration.

How to verify it

Run unit test.

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@ganglyu ganglyu marked this pull request as ready for review December 18, 2023 02:21
wen587
wen587 previously approved these changes Dec 18, 2023
with open(GOLDEN_CFG_FILE) as f:
golden_data = json.load(f)
if namespace is None:
self.minigraph_data = golden_data
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 18, 2023

Choose a reason for hiding this comment

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

minigraph_data

It's confusing to change the meaning of "minigraph_data". Suggest make it immutable, and create new var for new purpose.

If both GOLDEN_CFG_FILE and MINIGRAPH_FILE exists, the table inside GOLDEN_CFG_FILE will override table from MINIGRAPH_FILE. Please check with @wen587 and try reuse this override logic. #Closed

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 comment

wen587
wen587 previously approved these changes Dec 18, 2023
wen587
wen587 previously approved these changes Dec 18, 2023
wen587
wen587 previously approved these changes Dec 19, 2023

def update_config(current_config, config_input):
"""
Override current config with golden config
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 19, 2023

Choose a reason for hiding this comment

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

Override

Thanks for improving the code comments! #Closed

@qiluo-msft qiluo-msft self-requested a review December 20, 2023 00:03
# this is to avoid duplicating the hardcoded these values in db_migrator
self.config_src_data = None
if self.__minigraph_data:
self.config_src_data = self.__minigraph_data
Copy link
Contributor

@qiluo-msft qiluo-msft Dec 20, 2023

Choose a reason for hiding this comment

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

__minigraph_data

This is shallow copy. So any future change on right side will also appear on left side.
I understand it is not efficient to make minigraph_data immutable. So instead, you can call it config_src_data, and override it.

__xxx convention does not help your case, it is just make the member super private. #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.

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reused function, add an argument to update_config, and update_config will use deepcopy by default, but db migrator will use shallow copy.
For db migrator, add a method to generate config_src_data. golden_config_data and minigraph_data are local variables, so it's impossible to alter them and then we can prevent unintended change.

@ganglyu ganglyu merged commit bcb10f1 into sonic-net:master Dec 22, 2023
5 checks passed
mssonicbld pushed a commit to mssonicbld/sonic-utilities that referenced this pull request Jan 30, 2024
What I did
Need to support golden config in db migrator.

How I did it
If there's golden config json, read from golden config instead of minigraph.
And db migrator will use golden config data to generate new configuration.

How to verify it
Run unit test.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #3136

mssonicbld pushed a commit that referenced this pull request Jan 30, 2024
What I did
Need to support golden config in db migrator.

How I did it
If there's golden config json, read from golden config instead of minigraph.
And db migrator will use golden config data to generate new configuration.

How to verify it
Run unit test.
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.

5 participants