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: sort endpoint IP addresses before diffing #160

Merged
merged 4 commits into from
Oct 13, 2020

Conversation

wtam2018
Copy link
Contributor

Description:

This PR fixes argoproj/argo-cd#1816. For an Endpoint, unsorted/out of lexicographical order IP addresses in the desired state won't match the actual live state and the application won't able to move to "Synced" state. This PR adds normalize function to sort IP addresses before performing diff so that diff is successful and the application can go to "Synced" state.

@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #160 into master will decrease coverage by 0.07%.
The diff coverage is 40.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   48.21%   48.14%   -0.08%     
==========================================
  Files          36       36              
  Lines        2833     2858      +25     
==========================================
+ Hits         1366     1376      +10     
- Misses       1330     1339       +9     
- Partials      137      143       +6     
Impacted Files Coverage Δ
pkg/diff/diff.go 61.70% <40.74%> (-1.79%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d99997...338c60a. Read the comment docs.

Copy link
Contributor

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you @wtam2018 !

@@ -19,10 +19,17 @@ import (
"k8s.io/apimachinery/pkg/util/strategicpatch"
"k8s.io/client-go/kubernetes/scheme"

"k8s.io/kubernetes/pkg/api/endpoints"
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: can you please group third party dependencies together?

@alexmt
Copy link
Contributor

alexmt commented Oct 13, 2020

@wtam2018 resolve merge conflicts please

@ash2k
Copy link
Member

ash2k commented Oct 13, 2020

It's unfortunate that we're adding more dependencies on the k/k repo. #56

@sonarcloud
Copy link

sonarcloud bot commented Oct 13, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
26.5% 26.5% Duplication

@wtam2018 wtam2018 requested a review from alexmt October 13, 2020 23:19
@alexmt alexmt merged commit a1dc4c5 into argoproj:master Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diff on Endpoint confused by IP sorting
4 participants