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(cli): Included new info command #1230

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jamesgeorge007
Copy link
Contributor

Description

Included a new info command which shows debugging information about the environment which proves to be useful while submitting bug reports.

Sample Commands

firebase info

Environment Info:

  System:
    OS: Linux 4.18 Ubuntu 18.10 (Cosmic Cuttlefish)
    CPU: (4) x64 Intel(R) Core(TM) i5-7200U CPU @ 2.50GHz
  Binaries:
    Node: 8.11.4 - /usr/bin/node
    Yarn: 1.16.0-0 - /usr/local/bin/yarn
    npm: 6.9.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 74.0.3729.108
    Firefox: 66.0.3
  npmGlobalPackages:
    firebase: 6.7.2

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label May 1, 2019
@jamesgeorge007
Copy link
Contributor Author

@bkendall Thoughts 🤔

@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented May 1, 2019

The CI test fails as I have used async/await which it (eslint) doesn't expect. The envinfo.run() executes in an asynchronous manner and hence, one should wait until it resolves.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.557% when pulling 90a986d on jamesgeorge007:feat/info-cmd into aa39da5 on firebase:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 61.557% when pulling 90a986d on jamesgeorge007:feat/info-cmd into aa39da5 on firebase:master.

@coveralls
Copy link

coveralls commented May 1, 2019

Coverage Status

Coverage remained the same at 65.054% when pulling ec0f3b9 on jamesgeorge007:feat/info-cmd into 790209c on firebase:master.

Copy link
Contributor

@bkendall bkendall left a comment

Choose a reason for hiding this comment

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

  1. As this adds a new command, there's more internal process that has to happen as we (Firebase) are very deliberate about what we add to our public APIs, CLI commands included.

  2. This seems like it accomplishes a very similar goal to our output when the --debug flag is turned on. As "run the command with the --debug flag and provide the output" is a standard procedure for us while debugging, it may make more sense to include some of this information in that output instead. (And, as a bonus, if it's not adding a new command the friction to add this information output will be a lot lower.) What do you think?

@bkendall
Copy link
Contributor

bkendall commented May 1, 2019

@bkendall Thoughts 🤔

Did you read (2) in my previous comment?

@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented May 7, 2019

@bkendall IMO having a dedicated command to show up all those local environment information makes sense rather than including it with the --debug option 🤔
Most scaffolding tools these days are shipped with a dedicated info command which turns out to be helpful when the user might want to submit a bug.

@bkendall
Copy link
Contributor

bkendall commented May 7, 2019

@bkendall IMO having a dedicated command to show up all those local environment information makes sense rather than including it with the --debug option 🤔
Most scaffolding tools these days are shipped with a dedicated info command which turns out to be helpful when the user might want to submit a bug.

Noted, thank you. That still doesn't change (1) from above - for a new command, we have to do further review.

Do know that a lot of us are very busy with Google I/O related things, so it's unlikely anything changes this week. I can't say when an info command review would complete, but it will be measured in weeks not days.

Thanks for your contribution, but it does take time to get all the pieces together when adding a new command... thanks for your patience.

@jamesgeorge007
Copy link
Contributor Author

@bkendall I can understand 😃

@jamesgeorge007 jamesgeorge007 changed the title feat(cli): Added new info command feat(cli): Included new info command Jun 29, 2019
@jamesgeorge007
Copy link
Contributor Author

@bkendall Guess we're good to go after #105 gets merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants