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

Don't enqueue ingress for some service changes #365

Merged
merged 2 commits into from
Sep 10, 2018

Conversation

isaachawley
Copy link
Contributor

@isaachawley isaachawley commented Sep 7, 2018

When a service changes we get a service change event and an endpoint change event. Previously we'd enqueue the associated ingresses for all service changes. Now we only enqueue it when the service changes in a way that won't be covered by the endpoint changes.

Specifically for Service.spec.selector changes, which users sometimes use to do blue/green deployments, we will no longer enqueue an ingress - instead the changes will be handled (correctly) by the endpoint handler.

Fix for #306.

Bonus: Fewer reloads.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

isaac added 2 commits September 7, 2018 11:38
We only want to sync ingresses for service changes if particular
properties of the service are changed. We'll ignore others. DeepEqual
was causing us to sync ingresses when it was not necessary.
@isaachawley isaachawley added the bug An issue reporting a potential bug label Sep 7, 2018
@isaachawley isaachawley self-assigned this Sep 7, 2018
@isaachawley isaachawley merged commit d100319 into master Sep 10, 2018
@isaachawley isaachawley deleted the skip-enqueue-ingress branch September 10, 2018 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue reporting a potential bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants