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

feat: Add support for throwing instead of process.exit(1) on errors #118

Merged
merged 4 commits into from
Aug 1, 2018

Conversation

charlespwd
Copy link
Contributor

@charlespwd charlespwd commented Jul 17, 2018

Summary

When using the package as a library, it's a good idea to throw an error
instead of exiting with code 1. This way the user can recover from the
mistake somehow (e.g. deleting a throwaway environment on during continuous
integration).

This seems like it solves #111 too.

Description

  • When using the runMigration export, make the function throw instead of process.exit(1).
  • When using the default export, proceed as before by running process.exit(1) on errors.

Motivation and Context

I'm making continuous integration scripts that creates and deletes throwaway branches on CircleCI to test that the migration scripts are valid before they are merged. When the migration fails, I would like it to delete the newly created branch. This is currently impossible because this library will process.exit(1) for me.

Actual run-migration code:

async function main() {
  let statusCode = 0;
  try {
    await createEnvironment();
    await runMigrations();
  } catch (e) {
    console.error(e);
    statusCode = 1;
  } finally {
    await deleteEnvironment();
  }

  process.exit(statusCode);
}

Without this PR, the code above is unable to execute the finally block after errors.

I'm open to feedback / suggestions on how to improve this.

When using the package as a library, it's a good idea to throw an error
instead of exiting with code 1. This way the user can recover from the
mistake somehow (e.g. deleting a throwaway branch on during continuous
integration).
@charlespwd
Copy link
Contributor Author

I don't know how to configure the CONTENTFUL_INTEGRATION_* on travis. But the tests pass for me!
https://pastebin.com/raw/4sc5bWAC

@charlespwd charlespwd changed the title Add support for throwing instead of process.exit(1) on errors feat:Add support for throwing instead of process.exit(1) on errors Jul 19, 2018
@charlespwd charlespwd changed the title feat:Add support for throwing instead of process.exit(1) on errors feat: Add support for throwing instead of process.exit(1) on errors Jul 19, 2018
@Khaledgarbaya
Copy link
Contributor

Hi @charlespwd,
Thank you for the PR, we will do the test locally and come back to you. I am working on improving the integration test situation using fixtures which will solve the testing problem soon.

@charlespwd
Copy link
Contributor Author

Hi @Khaledgarbaya, @axe312ger. Any updates on this? We'd like to use this here at ALDO group. Thanks.

@Khaledgarbaya
Copy link
Contributor

Hi @charlespwd I will check this this first thing tomorrow.
Thanks

Copy link
Collaborator

@axe312ger axe312ger left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@axe312ger axe312ger left a comment

Choose a reason for hiding this comment

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

Loogs good but we need tests to make sure this behaves as expected and stays that way :)

@Khaledgarbaya
Copy link
Contributor

Hi @charlespwd,
First look the PR is looking good but we would like to have some tests for this behaviour. Especially some integration test. You can check the existing tests and copy some code from there to get a boilerplate ready.
Let me know if I can help
Khaled

@charlespwd
Copy link
Contributor Author

charlespwd commented Jul 31, 2018

Hey @Khaledgarbaya, the e2e tests fail for me on master. Is that normal?

     Error: Nock: No match for request {
  "method": "delete",
  "url": "https://api.contentful.com/spaces/hbs7843b5zgw/environments/env-integration",
  "headers": {
    "accept": "application/json, text/plain, */*",
    "content-type": "application/vnd.contentful.management.v1+json",
    "x-contentful-user-agent": "app contentful.migration-cli.e2e-test/0.11.0; sdk contentful-management.js/5.2.1; platform node.js/v8.9.4; os Linux/4.17.10-1-ARCH;",
    "authorization": "Bearer CFPAT-REDACTED",
    "user-agent": "node.js/v8.9.4",
    "accept-encoding": "gzip"
  }
}

@charlespwd
Copy link
Contributor Author

charlespwd commented Jul 31, 2018

So I figured out why.

  1. I think we're creating throwaway environment with the tests and I can't create more than 2 on a trial account.
  2. As a result, the env-integration environment doesn't exist.

@charlespwd charlespwd reopened this Jul 31, 2018
@charlespwd
Copy link
Contributor Author

OK, I got an e2e test in. Let me know if you need anything else :)

@Khaledgarbaya @axe312ger

Copy link
Contributor

@Khaledgarbaya Khaledgarbaya left a comment

Choose a reason for hiding this comment

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

Tested the test locally and it works fine.
Thanks, @charlespwd.

@Khaledgarbaya
Copy link
Contributor

available in v0.11.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants