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

[PR] Add a warning if patch doesn't succeed #339

Closed
5 tasks done
kopf-archiver bot opened this issue Aug 18, 2020 · 1 comment
Closed
5 tasks done

[PR] Add a warning if patch doesn't succeed #339

kopf-archiver bot opened this issue Aug 18, 2020 · 1 comment
Labels
archive enhancement New feature or request

Comments

@kopf-archiver
Copy link

kopf-archiver bot commented Aug 18, 2020

A pull request by michaelnarodovitch at 2020-04-01 12:30:01+00:00
Original URL: zalando-incubator/kopf#339
 

What do these changes do?

The situation

  • kopf submits a merge-patch+json patch
  • kubernetes API doesn't apply the merge-patch+json patch but responds 200 OK

leads to bugs and confusion, when testing operators.

With this change, kopf will check, if the merge-patch+json patch was merged and be verbose, if the patch was not exactly successful.

Description

Before: When a patch wasn't merged, then kopf didn't report anything.

After: When a patch wasn't merged, then kopf will report if ran in --verbose mode.

This will help users to debug their operators. Especially, when they use OpenAPI schemas in their CRDs.

Not sure if the check is in the right place and the logger is setup appropriately.

No risky changes.

Issues/PRs

If a patch is not merged, then it is usually because of a schema mismatch of the CRD. For this reason it was discussed to move the internal kopf status to the annotations. #308 #321

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • The code addresses only the mentioned problem, and this problem only
  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt

Commented by michaelnarodovitch at 2020-04-03 14:26:15+00:00
 

Thx for all the comments and suggestions. Had to add quite a bit of code. I think quality is OK and changes are safe.

Missing is integration tests with the Kubernetes API responses, and especially integration tests with the status_patch merging. What I did was based of an educated guess from reviewing https://github.com/kubernetes/apiextensions-apiserver. If we wanted to be 100% safe, we would also have to test the PATCH API of the canonical Kubernetes objects.

@kopf-archiver kopf-archiver bot closed this as completed Aug 18, 2020
@kopf-archiver kopf-archiver bot changed the title [archival placeholder] [PR] Add a warning if patch doesn't succeed Aug 19, 2020
@kopf-archiver kopf-archiver bot added the enhancement New feature or request label Aug 19, 2020
@kopf-archiver kopf-archiver bot reopened this Aug 19, 2020
@nolar
Copy link
Owner

nolar commented Sep 4, 2020

Reimplemented in #527 based on the original solution in this PR, but with a different approach to comparison (via diffs instead of own implementation of patching, which can mismatch with K8s's implementation).

@nolar nolar closed this as completed Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archive enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant