-
Notifications
You must be signed in to change notification settings - Fork 401
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
Bug 2034364: Use resource{apply,merge,read} functions provided by library-go #2833
Bug 2034364: Use resource{apply,merge,read} functions provided by library-go #2833
Conversation
Skipping CI for Draft Pull Request. |
@kikisdeliveryservice how would I go about verifying this doesn't break anything? Looked at some of the functions in |
This one is a bit harder to test since we never really had the full scope of unit testing for these libraries. We could attempt to add MCO specific field testing for this, though, just to make sure we don't break anything. Two ways we could approach this:
|
8e3fb5a
to
17392c5
Compare
depends on: #2837 |
52cb262
to
cd4437b
Compare
2a89f75
to
fb4a508
Compare
/test images |
/retest-required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is in good shape, defer to Jerry for final review
/assign @yuqi-zhang
/lgtm |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
skipping optional test to save bot keeping retesting |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
@mkenigs: This pull request references Bugzilla bug 2034364, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (rioliu@redhat.com), skipping review request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Putting hold until bugzilla and commit message is updated with with bug fix this PR does. |
@mkenigs: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Sorry for the delay, updated bugzilla description. If that looks good, feel free to remove hold. Thanks! |
Thanks Jerry! |
/test e2e-agnostic-upgrade |
@mkenigs: All pull requests linked via external trackers have merged: Bugzilla bug 2034364 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Currently we're getting the error: Error creating event &Event... Event ".16c2ed25cd8773b4" is invalid: involvedObject.namespace: Invalid value: "": does not match event.namespace Related to Bug 2034364, overlooked in openshift#2833
Borrowing from #829 which helps #819
We currently have code duplicated from library-go in ./lib, and this
commit uses some of the library-go functions instead and removes the
files that contained the duplicated code in
./lib/resource{apply,merge,read}.
Doing so requires adding events.Recorder as libgoRecorder to the
Operator struct.
Note that this does not remove all functions from lib; some lib
functions contain MCO specific logic or would not be trivial to move, so
doing so can be done at another time.
Unit testing these changes is not really possible since the newly used
functions are from outside the MCO repo, and the complexity of e2e
testing is not worth the effort at this time, since the library-go
functions should maintain current behavior. A more thorough test plan
might be necessary to move the rest of lib.