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

Break PKS Loop #277

Merged
merged 6 commits into from
Oct 31, 2018
Merged

Break PKS Loop #277

merged 6 commits into from
Oct 31, 2018

Conversation

maierlars
Copy link
Contributor

Check more carefully if an update is required to prevent update loops on PKS <3

@maierlars maierlars self-assigned this Oct 26, 2018
@maierlars maierlars requested a review from neunhoef October 26, 2018 08:06
@ghost ghost added the 2 - Working label Oct 26, 2018
Copy link
Member

@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

LGTM. Except that I believe one more function can remove the force argument.

return false
}

for i := 0; i < len(cl); i++ {
Copy link
Member

Choose a reason for hiding this comment

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

Might have to ignore the order.

func (c Condition) Equal(other Condition) bool {
return c.Type == other.Type &&
c.Status == other.Status &&
c.LastUpdateTime.Time.Sub(other.LastUpdateTime.Time).Seconds() < 2 &&
Copy link
Member

Choose a reason for hiding this comment

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

Use a function for time comparison. Also make sure we do not have to take the absolute value.

Copy link
Member

Choose a reason for hiding this comment

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

Add a test to make sure.

ds.Reason == other.Reason &&
ds.ServiceName == other.ServiceName &&
ds.SyncServiceName == other.SyncServiceName &&
reflect.DeepEqual(ds.Images, other.Images) &&
Copy link
Member

Choose a reason for hiding this comment

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

Get rid of DeepEqual altogether round here.

Copy link
Member

@neunhoef neunhoef left a comment

Choose a reason for hiding this comment

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

LGTM

@neunhoef neunhoef merged commit 5f786f6 into master Oct 31, 2018
@ghost ghost removed the 2 - Working label Oct 31, 2018
@neunhoef neunhoef deleted the bug-fix/break-pks-loop branch October 31, 2018 16:18
@maierlars maierlars mentioned this pull request Nov 2, 2018
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.

2 participants