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

fix ovn-bridge-mappings deletion #2564

Merged
merged 1 commit into from Mar 28, 2023

Conversation

zhangzujian
Copy link
Member

Which issue(s) this PR fixes:

Fixes incorrect ovn-bridge-mappings after removing a non-existent entry.

@zhangzujian zhangzujian added bug Something isn't working need backport labels Mar 28, 2023
@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative.
  • There are some unused variables in the code, they should be removed to improve readability.
  • The function ovsCleanProviderNetwork has a lot of duplicated code. It can be refactored to reduce the number of lines and make it more readable.
  • The function updateOvnMapping is not used anymore and can be removed.
  • The function decodeOvnMappings can be simplified by using strings.SplitN instead of strings.IndexRune.
  • The function encodeOvnMappings can be simplified by using strings.Builder.
  • The function getOvnMappings can return an empty map instead of nil when there is an error.
  • The function setOvnMappings can be simplified by using if len(mappings) == 0 instead of if s := encodeOvnMappings(mappings); len(s) == 0.
  • The function addOvnMapping can be simplified by using if mappings[key] == value || !overwrite { return nil }.
  • The function removeOvnMapping can be simplified by using if len(mappings) == length { return nil }.
  • The function initProviderChassisMac can be simplified by using addOvnMapping instead of ovs.Exec.

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative.
  • There are no potential bugs in the code patch diff.
  • The format of the code is consistent with the rest of the project.
  • There are some performance issues that can be improved. For example, the function getOvnMappings can be optimized to avoid unnecessary string operations.
  • The code can be further improved by using constants for the external-ids names.
  • The function addOvnMapping should have a better name since it can also update an existing mapping.
  • The function removeOvnMapping should return an error if the key does not exist in the mappings.

@zhangzujian zhangzujian merged commit 8e03e97 into kubeovn:master Mar 28, 2023
50 checks passed
@zhangzujian zhangzujian deleted the fix-ovn-mappings branch March 28, 2023 06:22
zhangzujian added a commit that referenced this pull request Mar 28, 2023
zhangzujian added a commit that referenced this pull request Mar 28, 2023
zhangzujian added a commit that referenced this pull request Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working need backport
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants