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 error when missing .release-tool/auth.yml #69

Merged
merged 1 commit into from
Dec 8, 2020

Conversation

tlsvda
Copy link
Contributor

@tlsvda tlsvda commented Nov 23, 2020

[#61]

It has to show a cleaner error when .release-tool/auth.yml is missing.

@tlsvda tlsvda requested a review from a team November 27, 2020 14:41
throw new RuntimeException(
sprintf('The file %s needs to exist and contain a GitHub access token', $configFile)
);
exit(sprintf('The file %s needs to exist and contain a GitHub access token.' . PHP_EOL, $configFile));
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we should allow the file to be created when it does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid that just creating the file doesn't help that much, because a couple lines down the config file is being processed and afterwards the code is expecting that $processedConfiguration['github']['token'] is available.

Maybe adding a release-tool config credentials.github XXXXXXXXXXXXXXXXXXXXXXX command, just like composer has, will offer a nice developer experience. In that case you can keep the error message with the exit and just refer to the config command to properly configure the credentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. I meant to interactively generate the file and ask for the token.
But maybe directing the user to another command is cleaner.

throw new RuntimeException(
sprintf('The file %s needs to exist and contain a GitHub access token', $configFile)
);
exit(sprintf('The file %s needs to exist and contain a GitHub access token.' . PHP_EOL, $configFile));
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels like a HTTP 200 OK response with a error message in the body. This error message is way better than a complete exception stack trace but we should not pretend that nothing happend and stop with a clean exit.

At least a exit(1) (https://tldp.org/LDP/abs/html/exitcodes.html) should be used to let the terminal, and user, know that an error has occurred.

@tlsvda tlsvda force-pushed the fix-error-missing-auth-yaml branch from b2a327d to ca2441b Compare December 4, 2020 09:25
[#61]

It has to show a cleaner error when .release-tool/auth.yml is missing.
@tlsvda tlsvda merged commit 716b118 into master Dec 8, 2020
@tlsvda tlsvda deleted the fix-error-missing-auth-yaml branch December 8, 2020 07:50
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.

None yet

3 participants