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

fully process the endpoints object before returning error #519

Merged
merged 3 commits into from
May 19, 2021

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented May 18, 2021

Changes proposed in this PR:

  • Allow the endpoints controller to fully process the endpoints object prior to returning error in the case where processing one of the target addresses fails.

How I've tested this PR:
unit test added which creates an address which does not have a backing pod so that when we reconcile it that address fails and then we assert that the others still got processed.

How I expect reviewers to test this PR:
code review + unit tests
Note to reviewers: I thought about adding tests cases which cover other errors in the reconcile path aside from what is covered in the TestReconcileCreateEndpoints() but I'm not sure they are totally necessary to validate the code changes. I'd be happy to add additional test cases to other tests of course.

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@kschoche kschoche self-assigned this May 18, 2021
@kschoche kschoche requested review from a team, ndhanushkodi and thisisnotashwin and removed request for a team May 18, 2021 20:41
@kschoche kschoche added the theme/tproxy Items related to transparent proxy label May 18, 2021
@kschoche kschoche marked this pull request as ready for review May 18, 2021 20:41
Copy link
Contributor

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

great job on this one @kschoche!!

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Yup I think the test case in the "create" test makes sense since it covers the create/update logic paths and this doesn't apply for delete. Nice work!!!

@kschoche kschoche merged commit 9f7350e into master May 19, 2021
@kschoche kschoche deleted the tproxy-endpoints-controller-continue-on-error branch May 19, 2021 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/tproxy Items related to transparent proxy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants