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

Faucet improvements (fix bug #961) #1048

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

wd021
Copy link
Contributor

@wd021 wd021 commented Feb 19, 2022

Improve Faucet

  • email should be a required flag in the CLI prompt (because it is required by the client)
  • Too many faucet requests and You entered an invalid email. errors result in error Command failed with exit code 1., as seen in Bug #961. Improve error handling to exit gracefully here.

Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.

[ ] Yes
[x] No

@wd021 wd021 requested a review from a team as a code owner February 19, 2022 09:31
@wd021 wd021 changed the title Faucet improvements Faucet improvements (fix bug #961) Feb 19, 2022
@wd021 wd021 mentioned this pull request Feb 19, 2022
Closed
@@ -41,7 +41,7 @@ export class FaucetCommand extends IronfishCommand {

if (!email) {
email = (await CliUx.ux.prompt('Enter your email to stay updated with Iron Fish', {
required: false,
required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Email isn't required, as it uses your public key if you don't specify an email.

Copy link
Contributor Author

@wd021 wd021 Feb 23, 2022

Choose a reason for hiding this comment

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

Oh oops, you're right. I thought it was required cause leaving email blank results in Collecting your funds... You entered an invalid email.. Turns out it submits an empty string to the API, which always fails. So we need to pass undefined if a user skips email.

Updated the PR for this.

@NullSoldier
Copy link
Contributor

NullSoldier commented Feb 22, 2022

This PR creates a bug in that an errored command now returns an exit code 0. I do agree the error that yarn outputs is really annoying. We manage and print our own errors, so we don't need yarns. It's actually expected that non zero exit codes are returned in CLI commands, or else you can't use the CLI for programatic purposes, and you could have to artificially return 0 in all commands which we don't do.

There is unfortunately no way in yarn to suppress this for individual commands. We could use --silent in a .yarnrc but it would suppress that for build, install, and all the other commands too. This is purely an issue with people using engineering tooling to run their node, and unfortunately having to deal with the usability of using a build tool to just run the software from source.

@wd021
Copy link
Contributor Author

wd021 commented Feb 23, 2022

Yea, that's a bit annoying. I think adding --silent in .yarnrc in the ironfish-cli folder is the best resolution. If necessary, perhaps add a line in the documentation for this.

@@ -0,0 +1 @@
--silent true
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the tricky thing I was trying to point out. If you do this, it breaks all the logs for developers using yarn commands for install, build, etc because nothing is printed. Honestly, I think if you're using from source you have to be prepared to use engineering tooling and know how it works and be okay with these errors.

We're going to release NPM builds soon so 90% of users won't be running from source anymore and therefore should not be running yarn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha. yea, I'm not too concerned with the current error messaging. will just leave bug #961 as is then.

@NullSoldier
Copy link
Contributor

This looks good, merging!

@NullSoldier NullSoldier merged commit 2628be2 into iron-fish:staging Mar 7, 2022
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

2 participants