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

Replace failure crate with anyhow+thiserror #286

Conversation

steveej
Copy link
Contributor

@steveej steveej commented Jun 4, 2020

Failure was deprecated and suggests anyhow+thiserror to replace
itself in dependent projects.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2020
@steveej steveej force-pushed the pr/migrate-away-from-failure branch from 9c6dc94 to 349868e Compare June 4, 2020 15:56
@steveej
Copy link
Contributor Author

steveej commented Jun 4, 2020

CI flake, Images failed to build.

/retest

@@ -1,4 +1,4 @@
use failure::Fallible;
use anyhow::Result as Fallible;
Copy link
Member

Choose a reason for hiding this comment

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

Cheater 😉

Failure was [deprecated][0] and suggests anyhow+thiserror to replace
itself in dependent projects.

[0]: rust-lang-deprecated/failure#347
@steveej steveej force-pushed the pr/migrate-away-from-failure branch from 349868e to b138286 Compare June 4, 2020 17:17
@steveej
Copy link
Contributor Author

steveej commented Jun 4, 2020

CI flake, Images failed to build.

I was mislead by this message:

   * could not run steps: step e2e-test failed: could not wait for build: the build e2e-test failed with reason DockerBuildFailed: Docker build strategy has failed. 

when in fact it was an unresolved import in the e2e tests which I missed when testing locally.

@@ -154,25 +151,27 @@ pub struct Empty;

/// Errors that can be returned by the methods in this library
pub mod errors {
use commons::prelude_errors::*;
Copy link
Member

Choose a reason for hiding this comment

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

I have not seen this before. Can you please explain purpose of this?

Copy link
Member

Choose a reason for hiding this comment

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

Also did not find any pointers in internet e.g. https://doc.rust-lang.org/std/prelude/index.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prelude is a naming convention for modules which export commonly used items. AFAIU there's nothing right or wrong about it, and I decided to include errors in the name as this prelude is about error related items only.

Copy link
Member

@LalatenduMohanty LalatenduMohanty 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 openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 9, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, LalatenduMohanty, steveeJ

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:
  • OWNERS [LalatenduMohanty,steveeJ]

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 22c66f1 into openshift:master Jun 9, 2020
@steveej steveej deleted the pr/migrate-away-from-failure branch June 10, 2020 12:15
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

5 participants