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 2034484: Library go bump #434
Bug 2034484: Library go bump #434
Conversation
e6da711
to
59c3527
Compare
/retest-required |
Unit tests are failing at this point, there were some changes with the @joelddiaz is there any context you can provide on the expected behavior?
|
70d5915
to
2998451
Compare
Codecov Report
@@ Coverage Diff @@
## master #434 +/- ##
==========================================
- Coverage 46.35% 45.41% -0.94%
==========================================
Files 92 92
Lines 9206 9229 +23
==========================================
- Hits 4267 4191 -76
- Misses 4418 4532 +114
+ Partials 521 506 -15
|
/retest-required |
1 similar comment
/retest-required |
@eggfoobar #438 has merged so you should re-base on top of that/master now |
updated library go to latest Signed-off-by: ehila <ehila@redhat.com> upkeep: ran make update Signed-off-by: ehila <ehila@redhat.com>
Signed-off-by: ehila <ehila@redhat.com>
Signed-off-by: ehila <ehila@redhat.com> fix: unit test for cleanup stale state Fake client now respects finalizers, if any are present delete functionality will simulate real cluster resource delete More info: kubernetes-sigs/controller-runtime#1399 Signed-off-by: ehila <ehila@redhat.com> fix: updated unit tests mock object with max call Signed-off-by: ehila <ehila@redhat.com>
2998451
to
0212d37
Compare
/retest-required |
2 similar comments
/retest-required |
/retest-required |
@eggfoobar: This pull request references Bugzilla bug 2034484, which is valid. 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 (jhou@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. |
/retest-required |
/retest-required |
/assign @joelddiaz |
pkg/aws/actuator/actuator_test.go
Outdated
} else { | ||
fakeClient = fake.NewFakeClient() | ||
fakeClient = fake.NewClientBuilder().WithRuntimeObjects().Build() |
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.
Is
fakeClient = fake.NewClientBuilder().WithRuntimeObjects().Build() | |
fakeClient = fake.NewClientBuilder().Build() |
a legitimate way to spell this?
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.
Good catch, updated to remove redundant builder function call
/test e2e-gcp |
@@ -287,7 +287,7 @@ func testPassthroughCredentialsRequest(t *testing.T) *minterv1.CredentialsReques | |||
ObjectMeta: metav1.ObjectMeta{ | |||
Name: testStaleCRName, | |||
Namespace: testNamespace, | |||
Finalizers: []string{minterv1.FinalizerDeprovision}, | |||
Finalizers: []string{}, |
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.
Rather than this, can we leave the Finalizer, but change the codepath that verifies deltion from assert.Nil() to assert.NotNil(t, cr.DeletionTimestamp, ...) specifically:
- assert.Nil(t, cr, "expected credentials request to be deleted")
+ assert.NotNil(t, cr.DeletionTimestamp, "expected credentials request to be marked for deletion")
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.
Absolutely, updated
updated unit test to retain FinalizerDeprovision for cleanup_controller_test.go modified client builder to remove unneeded builder function call Signed-off-by: ehila <ehila@redhat.com>
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eggfoobar, joelddiaz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest-required Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest-required Please review the full test history for this PR and help us cut down flakes. |
@eggfoobar: 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. |
@eggfoobar: Bugzilla bug 2034484 is in an unrecognized state (ON_QA) and will not be 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. |
Bumping the library-go dependency to latest, this should take advantage of performance improvements in this PR.
Changes:
xref: https://issues.redhat.com/browse/CNF-2808