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

[yang] remove mistakenly added parameter for 'get_module_name' #2193

Merged
merged 2 commits into from
Jun 9, 2022

Conversation

ayurkiv-nvda
Copy link
Contributor

Signed-off-by: Andriy Yurkiv [email protected]

What I did

Revert parameter from function that was mistakenly added in #2118

We don't need to pass 'sonic_yang_options' to static function get_module_name()

How I did it

remove 'sonic_yang_options' from SonicYang object creation in get_module_name()

How to verify it

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)

@ayurkiv-nvda ayurkiv-nvda changed the title [yang] revert mistakenly added parameter for 'get_module_name' [yang] remove mistakenly added parameter for 'get_module_name' Jun 6, 2022
@liat-grozovik
Copy link
Collaborator

@ayurkiv-nvda please check coverage report and add missing unit tests

@ayurkiv-nvda
Copy link
Contributor Author

ayurkiv-nvda commented Jun 8, 2022

@ayurkiv-nvda please check coverage report and add missing unit tests

@liat-grozovik This PR just partially reverts some changes from recently merged PR https://github.com/Azure/sonic-utilities/pull/2118/files
We even didn't need this change ( that we revert here ) for the fix at all, it was added by mistake, that's why we revert.

So, eventually we change nothing in get_module_name() function, it just return to previous codebase when everything was good (before my change in #2118)
Checker that is failing - code coverage checker. There is no coverage at all for this part of code. And since eventually we don't change it, this part will be the same as before, so I believe it can be merged.

Anyway, If needed we can come up with some UT infra and test to improve code coverage for Yang config_manager, but we will need to invest more time.

@volodymyrsamotiy
Copy link
Collaborator

@qiluo-msft, could you please help merging this PR?

Coverage is failing because it is 0%. But in this PR we just revert 1 line of code that was changed recently by another PR. And before that coverage was 0% as well. So I think we can be safely merge with coverage 0% (because in fact we just move to previous state).

We need to revert this 1 line because it caused degradation in application extension installation flow (BTW we even didn't need this change in that another PR, it was changed by mistake).

Please check previous comment for some more details.

@qiluo-msft
Copy link
Contributor

Could you add a unit test to cover the flow of get_module_name()? The old code will fail on this unit test because it has python syntax error, but new code will pass.

@ayurkiv-nvda
Copy link
Contributor Author

Could you add a unit test to cover the flow of get_module_name()? The old code will fail on this unit test because it has python syntax error, but new code will pass.

@qiluo-msft I have updated PR, please take a look at new UT added to check flow of "get_module_name"

@qiluo-msft qiluo-msft merged commit bb185d5 into sonic-net:master Jun 9, 2022
judyjoseph pushed a commit that referenced this pull request Jun 13, 2022
#### What I did
Revert parameter from function that was mistakenly added in #2118

We don't need to pass 'sonic_yang_options' to static function get_module_name()

#### How I did it
remove 'sonic_yang_options' from SonicYang object creation in get_module_name()
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.

6 participants