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.

Sign up for GitHub

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

Jump to bottom

BUG 1824497: Enable error checks to unwrap errors #559

Merged
merged 2 commits into from Apr 17, 2020
Merged

BUG 1824497: Enable error checks to unwrap errors #559

merged 2 commits into from Apr 17, 2020

Conversation

JoelSpeed
Copy link
Contributor

openshift/cluster-api-provider-aws#309 introduced error wrapping around some of the errors in returned by the actuator. This meant that these checks for errors may no longer work depending on whether the error was wrapped or not.

In particular, any error on launching the instance was being wrapped and as such, the Machine was not going into the failed phase.

This should prevent this sort of thing in the future by updating the checks to unwrap errors before checking them against the given type.

This uses errors.As which unwraps until it finds an error of the given type, or returns false if the error is not an error or wrapped error of the given type

@openshift-ci-robot
Copy link
Contributor

@JoelSpeed: This pull request references Bugzilla bug 1824497, 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
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

BUG 1824497: Enable error checks to unwrap errors

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 17, 2020
@enxebre
Copy link
Member

enxebre commented Apr 17, 2020

nice
Please can you make sure to reference here the counter part PRs for actuators?
/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2020
@JoelSpeed JoelSpeed mentioned this pull request Apr 17, 2020
Merged
alexander-demicev
alexander-demicev approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

/lgtm

thanks a lot for this

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@JoelSpeed
Copy link
Contributor Author

/hold

I want to verify this before it merges

@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 Apr 17, 2020
@JoelSpeed
Copy link
Contributor Author

/hold cancel

I've verified this works to solve the bug in conjunction with openshift/cluster-api-provider-aws#316

@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 Apr 17, 2020
@openshift-bot
Copy link
Contributor

/retest

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

This was referenced Apr 17, 2020
Merged
Merged
@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 17, 2020 •
edited

@JoelSpeed: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-azure 367ae64 link /test e2e-azure
ci/prow/e2e-gcp 367ae64 link /test e2e-gcp
ci/prow/e2e-azure-operator 367ae64 link /test e2e-azure-operator

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@openshift-merge-robot openshift-merge-robot merged commit 1393bce into openshift:master Apr 17, 2020
@openshift-ci-robot
Copy link
Contributor

@JoelSpeed: An error was encountered searching for bug 1824497 on the Bugzilla server at https://bugzilla.redhat.com:

Get https://bugzilla.redhat.com/rest/bug/1824497?api_key=CENSORED: dial tcp: i/o timeout
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

BUG 1824497: Enable error checks to unwrap errors

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.

@JoelSpeed
Copy link
Contributor Author

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@JoelSpeed: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:

In response to this:

/bugzilla refresh

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.

@JoelSpeed JoelSpeed deleted the error-wraps branch April 20, 2020 09:24
@JoelSpeed JoelSpeed mentioned this pull request Apr 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-demicev alexander-demicev alexander-demicev approved these changes

@enxebre enxebre Awaiting requested review from enxebre

Assignees

@alexander-demicev alexander-demicev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@JoelSpeed @openshift-ci-robot @enxebre @openshift-bot @alexander-demicev @openshift-merge-robot