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

Fix failing unit tests in actuator remediation test #82

Merged
merged 1 commit into from Jul 9, 2020

Conversation

n1r1
Copy link

@n1r1 n1r1 commented Jul 6, 2020

@dhellmann reported that unit tests fails locally.

This PR fixes the failing test.

Signed-off-by: Nir <niry@redhat.com>
@n1r1
Copy link
Author

n1r1 commented Jul 6, 2020

/assign @dhellmann

@hardys
Copy link

hardys commented Jul 6, 2020

Thanks - this fixes the local failures I was also seeing:

$ go test .
ok  	github.com/openshift/cluster-api-provider-baremetal/pkg/cloud/baremetal/actuators/machine	0.046s

make unit still has some errors elsewhere but that looks related to my environment.

Do we know how/why this worked in CI but failed locally?

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 6, 2020
@n1r1
Copy link
Author

n1r1 commented Jul 6, 2020

make unit still has some errors elsewhere but that looks related to my environment.

I also have some errors and thought it's only on my end. This is the error I'm getting:

?       github.com/openshift/cluster-api-provider-baremetal/pkg/apis/baremetal  [no test files]
2020/07/06 14:30:21 failed to start the controlplane. retried 5 times: timeout waiting for process kube-apiserver to start

Do you get the same error?
Anyway, it happens even without this PR changes.

Do we know how/why this worked in CI but failed locally?

That's a good question.
It was discussed in #72 and I see that @kirankt created a ticket to track this, though I have no idea where do these tickets live.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@dhellmann
Copy link

While debugging this last week I noticed that there are a lot of client calls where errors aren't being checked. Does the test client return errors in the way the real client does?

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@dhellmann
Copy link

See also #80

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@dhellmann
Copy link

/hold

let's stop consuming CI resources in a loop

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2020
@n1r1
Copy link
Author

n1r1 commented Jul 6, 2020

While debugging this last week I noticed that there are a lot of client calls where errors aren't being checked. Does the test client return errors in the way the real client does?

I think it does.
I guess we can also check the errors from the fake client as well.
Would you like me to do this as part of this PR or a separate one?

@dhellmann
Copy link

While debugging this last week I noticed that there are a lot of client calls where errors aren't being checked. Does the test client return errors in the way the real client does?

I think it does.
I guess we can also check the errors from the fake client as well.
Would you like me to do this as part of this PR or a separate one?

Either way is fine.

@n1r1
Copy link
Author

n1r1 commented Jul 9, 2020

/test e2e-metal-ipi

@n1r1
Copy link
Author

n1r1 commented Jul 9, 2020

/unhold

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020
Copy link

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, hardys, n1r1

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit a6bf9e1 into openshift:master Jul 9, 2020
honza pushed a commit to honza/cluster-api-provider-baremetal that referenced this pull request Feb 7, 2022
📖 Update API docs for Metal3DataClaim objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants