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(process): fail if any of the steps fails #42

Merged
merged 1 commit into from
Jun 7, 2017

Conversation

mlazowik
Copy link
Contributor

@mlazowik mlazowik commented Jun 3, 2017

  • fix --post-success option description
  • add a new --mock-push option for testing purposes
  • add a test for successful post-success invocation

BREAKING CHANGE:
If pre-commit, git push or post-commit fails the whole program will also now fail with non-0 exit
code.

ISSUES CLOSED: #40

* fix --post-success option description
* add a new --mock-push option for testing purposes
* add a test for successful post-success invocation

BREAKING CHANGE:
If pre-commit, git push or post-commit fails the whole program will also now fail with non-0 exit
code.

ISSUES CLOSED: leonardoanalista#40
@coveralls
Copy link

Coverage Status

Coverage increased (+1.0%) to 93.416% when pulling a16b6b5 on qedsoftware:master into 2c6682c on leonardoanalista:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+1.0%) to 93.416% when pulling a16b6b5 on qedsoftware:master into 2c6682c on leonardoanalista:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.0%) to 93.416% when pulling a16b6b5 on qedsoftware:master into 2c6682c on leonardoanalista:master.

commitFeat();
const out = semanticRelease(`--post-success do-publish -v`);
const result = semanticRelease(`--post-success "npm run do-publish" -v`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't quite follow how this test is passing now. From what I understand, L385 will detect that a release is required and call git push. Does git push within this system test return 0 now (instead of 128)? Can you explain further, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sure. I've added an option to the whole command that tells it to mock the git push step and return specific code. Then I've added it to semanticRelease, so that it succeeds. Then if a test wants it to fail it can call semanticRelease with --mock-push 1, as options added later override those added earlier.

This is of course a very hacky solution, but I don't see any other possibility without more refactoring (see #41), which would be out of scope of this change.

@@ -373,8 +421,7 @@ describe('corp-semantic-release', function() {
// }

function semanticRelease(params) {
let result = shell.exec(`node ${__dirname}/../src/index.js ${params || ''}`);
return result.stdout;
return shell.exec(`node ${__dirname}/../src/index.js --mock-push 0 ${params || ''}`);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uglow see here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, that's what I missed. Thank you.

@@ -21,8 +21,13 @@ module.exports = function addFilesAndCreateTag(newVersion) {

// ###### PUSH CHANGES #####
log.info('>>...and push to remote...');
code = shell.exec('git push && git push --tags').code;
if (mockPush === undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@uglow and here

@uglow
Copy link
Collaborator

uglow commented Jun 5, 2017

Looks good to me 👍🏻

@uglow
Copy link
Collaborator

uglow commented Jun 7, 2017

@leonardoanalista Good to merge?

Copy link
Owner

@leonardoanalista leonardoanalista left a comment

Choose a reason for hiding this comment

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

Hi @mlazowik thanks for the PR. New file runScript.js is the one to wrap the scripts execution and terminate or not the process on fail. Thank you. 👍

@leonardoanalista leonardoanalista merged commit 48e9023 into leonardoanalista:master Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The command should fail if post-success fails
4 participants