Skip to content

Conversation

@TrCaM
Copy link
Contributor

@TrCaM TrCaM commented Jul 16, 2019

Description

Implement Create a new project option of firebase init command to actually create a new project instead of asking user to create the project with Firebase Console.

  • What changes: if the default project is not set
    • Ask a selection question with 3 choices asking for user's desired behavior: Use an existing project, Create a new project and Don't set up default project
    • Use and existing project: (completed)
      • If the logged in account has at most 100 projects, list all projects and let user select project from the list
      • If the logged in account has more than 100 projects, prompt user for the project ID of the desired Firebase projects
    • In this PR Create a new project
      • Reuse the flow from projects:create command to create a new project
      • Set the new project as the default project for the current directory
    • Don't set up default project => do nothing

API methods changes

  • Moved createFirebaseProject(), which implements the whole project creation flow and command line output, from commands/projects-create.ts to management/projects.ts since this process is reused in init/features/project.ts

Notes:

TODO:

  • Write unit test for createFirebaseProject() method which was moved into /management/projects.ts, once the approach is approved (Update: skipping testing since we already tested the underlying API methods)

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Jul 16, 2019
@TrCaM TrCaM changed the title Tc init create project opt Implement "Create a new project" option Jul 16, 2019
@TrCaM TrCaM requested review from bkendall, mbleigh, nbegley and rosecm July 16, 2019 15:43
@coveralls
Copy link

coveralls commented Jul 16, 2019

Coverage Status

Coverage decreased (-0.1%) to 63.115% when pulling 7e4d320 on tc-init-create-project-opt into bddf9b5 on tc-choose-project.

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.

I think things generally look fine in this PR... I'd ask @mbleigh for his opinion on where that code ends up as well though.

@TrCaM TrCaM requested review from bkendall and mbleigh July 17, 2019 21:25
@TrCaM
Copy link
Contributor Author

TrCaM commented Jul 18, 2019

@bkendall, @mbleigh, @nbegley. One last problem in this PR. createFirebaseProjectAndLog() is now a valid testing target. As you can remember, ora is a bit tedious to stub since the library export the function directly. A while ago I made an OraWrapper class to solve the problem, but IMO doing that just for testing is not ideal. In fact, I found proxyquire library can be a good solution if we agree to add an extra dependency to our repo

So the question is, can we just omit testing this? (I think we shouldn't). And if we decide to test, what're your opinions on testing ora?

@bkendall
Copy link
Contributor

Please don't add proxyquire - I've found it ends up promoting bad design....

This is one of those difficult choices - at some point, things aren't easily testable (especially when there's user input and logging involved), so we have to put some trusts in the tests we have and the manual testing we do. The fact that the function is exported doesn't make testing a requirement, technically. We definitely should have as many tests as we can, but at some point it becomes difficult.

Design-wise, this is why we try to keep as much of that logging in the Command files - those are already very difficult to test, but if we're testing the functional core behind it, we can be very confident that the command is going to be successful.

It's not the end of the world if we can't write tests for something. If the design can be changed so that better tests can be written, that's always preferred. But please, don't bring in proxyquire.

@TrCaM
Copy link
Contributor Author

TrCaM commented Jul 18, 2019

Sounds good to me that we can omit testing logging behavior here. I also believe the underlying tested api logic :). In fact the its logic is quite simple too.

@TrCaM TrCaM merged commit 7482250 into tc-choose-project Jul 19, 2019
@TrCaM TrCaM deleted the tc-init-create-project-opt branch July 19, 2019 13:41
TrCaM added a commit that referenced this pull request Jul 23, 2019
)

* Implement getFirebaseProject and deprecate firebaseApi

* Implement getProjectPage() and refactor listFirebaseProject()

* Implement use an existing project option

* Update behavior when there's no project

* Implement "Create a new project" option (#1510)
TrCaM added a commit that referenced this pull request Jul 29, 2019
)

* Implement getFirebaseProject and deprecate firebaseApi

* Implement getProjectPage() and refactor listFirebaseProject()

* Implement use an existing project option

* Update behavior when there's no project

* Implement "Create a new project" option (#1510)
TrCaM added a commit that referenced this pull request Aug 1, 2019
)

* Implement getFirebaseProject and deprecate firebaseApi

* Implement getProjectPage() and refactor listFirebaseProject()

* Implement use an existing project option

* Update behavior when there's no project

* Implement "Create a new project" option (#1510)
joehan added a commit that referenced this pull request Nov 13, 2019
* fix build container to have later npm (#1570)

* Release Firestore emulator v1.7.0 (#1571)

* Use ADC for Emulator Tests (#1575)

* Adds privacy policy link to login notice. (#1576)

* Use displayName when available (#1555)

* shows displayName if available

* formats

* uses displayName if available

* formats

* Declare project and app metadata types (#1470) and update dependencies

* Implement projects:create command (#1429)

* Implement api calls to support project creation

* Initial working command

* Replace nock with sinon to mock api requests

* export createFirebaseProject() and update tests

* Implement projects:create command

* Remove poll helper methods

* Rename firebase-resource-manager.ts to projectsCreate.ts

* Declare project and app metadata types

* Update timeout constant

* Update unit tests as suggestion

* Split metadata.ts into apps.ts and projects.ts

* move code to projects.ts

* Addressing comment

* Implement apps:create command (#1438)

* Implement API calls for apps:create command

* Implement apps:create command

* Remove --sha-certificate option

* Support providing appStoreId for iOS app creation

* Update the command to use getProjectId()

* Update unit tests as suggestion

* move code to apps.ts

* Add input options interfaces and getAppPlatform

* Implement projects:list command (#1465)

* Implement projects:list command

* Matching changes from metadata

* Move code to projects.ts

* Resolving TODO

* Implement apps:list command (#1477)

* Implement apps:list command

* Introducing AppPlatform.ANY and code refactoring

* Move code to apps.ts

* Implement getFirebaseProject and deprecate firebaseApi (#1502)

* Re-implement `firebase init` command's default project setup flow (#1506)

* Implement getFirebaseProject and deprecate firebaseApi

* Implement getProjectPage() and refactor listFirebaseProject()

* Implement use an existing project option

* Update behavior when there's no project

* Implement "Create a new project" option (#1510)

* [projects:list] and [apps:list] Logging count (#1526)

* Logging total project count after listing.

* Logging total app count

* Implement apps:sdkconfig command (#1515)

* Implement API methods for apps:sdkconfig

* Implement apps:sdkconfig command

* Add getOrPromptProject function to projects.ts (#1552)

* Make getAppPlatform throw if the input platform is unknown

* Update changelog for project and app management

* Move changelog into CHANGELOG.MD

* No more requireAuth() for functions (#1577)

* Add the ability to add Firebase resources to an existing GCP project (#1572)

* Implement projects:addfirebase command

* Update init command to have add Firebase option

* implements style fixes suggested in other pr

* typescriptifys ensureApiEnabled (#1554)

* typescriptifys ensureApiEnabled

This reverts commit e7ec80f.

* style fixes for switch to ts

* Release Firestore emulator v1.7.1 (#1584)
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.

7 participants