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: Validate required parameters after reading config files. #1928

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

Rob--W
Copy link
Member

@Rob--W Rob--W commented Jun 15, 2020

Fixes #1327.

@Rob--W Rob--W requested a review from rpl June 15, 2020 00:00
@coveralls
Copy link

coveralls commented Jun 15, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 0628b8e on Rob--W:required-params-from-config into 810f64c on mozilla:master.

@Rob--W Rob--W force-pushed the required-params-from-config branch from ae11b8a to 7aa95df Compare June 15, 2020 00:17
@Rob--W Rob--W changed the title Validate required parameters after reading config files. fix: Validate required parameters after reading config files. Jun 15, 2020
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@Rob--W thanks a lot for taking a look into this. I'm not super-thrilled about monkeypatching the yargs internals as a long-term solution but as a short-term workaround it can be fine (especially given the new tests added to catch regressions around that workaround).

There are a few nits that I would like to be handled on the test side (on the workaround implementation side I'm not requiring any actual changes, but it would be great if we could file a new issue to reference in the nearby inline comment, so that we may get back to this and see if we can think of a long-term solution that wouldn't need to monkeypatch yargs).

tests/functional/test.cli.sign.js Outdated Show resolved Hide resolved
tests/functional/test.cli.sign.js Outdated Show resolved Hide resolved
src/program.js Outdated Show resolved Hide resolved
@Rob--W Rob--W force-pushed the required-params-from-config branch from 7aa95df to 0628b8e Compare June 15, 2020 17:50
@rpl rpl added this to Backlog in 4.3.0 Jun 15, 2020
@rpl rpl merged commit 04b1bf5 into mozilla:master Jun 17, 2020
@rpl rpl moved this from Backlog to Done in 4.3.0 Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
4.3.0
Done
Development

Successfully merging this pull request may close these issues.

"Missing required arguments: api-key, api-secret" with correct configuration
3 participants