-
Notifications
You must be signed in to change notification settings - Fork 3
Seeder and Emulator Setup #155
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
base: main
Are you sure you want to change the base?
Conversation
| "https://securetoken.googleapis.com/v1/token"; | ||
| const FIREBASE_OAUTH_SIGN_IN_URL = | ||
| "https://identitytoolkit.googleapis.com/v1/accounts:signInWithIdp"; | ||
| const IDENTITYTOOLKIT_PASSWORD_PATH = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have not tested yet, but concerned about this change. Please explain your thought process (next time include this in PR description)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was strictly a matter of making things more readable, it's not a change that is necessary but I added for sake of usability in the future
|
Also,
|
sthuray
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't taken a closer look at code yet--would like to see clear documentation and refactoring before that.
Left some notes on what can be documented/refactored, please use your best judgement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document code blocks for clarity
|
|
||
| const auth = admin.auth(); | ||
|
|
||
| const USERS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Place mock data elsewhere and then import for easy editing of mock data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do and will do!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document purpose of file somewhere
Idea: in seeders, create a mock data folder that holds .json files. Then add a documentation file in the folder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one isn't necessarily needed as it will generate when testing, I can include this in the .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document file + rename functions for clarity
| const EMULATOR_HOST = process.env.FIREBASE_AUTH_EMULATOR_HOST; // e.g. "host.docker.internal:9099" | ||
| const USING_EMULATOR = Boolean(EMULATOR_HOST); | ||
| const API_KEY = USING_EMULATOR | ||
| ? process.env.FIREBASE_WEB_API_KEY || "fake-api-key" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is "fake-api-key" still suppposed to be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, left it as a place-holder for now, was planning to remove after testing went well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document purpose of each emulator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Document purpose of firebase-emulator service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I have documentation for this already on the notion ticket itself, but I can make it more thorough if need be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.gitignore
|
lgtm |
Changed pets seeder to delete all pets instead of filtering by hardcoded names from fixtures. This prevents accidental deletion of real pets that share names with seed data.
Notion ticket link
Seeding Script
Builds on from #143 and 53391bf.
Implementation description
config.jsfile on the backend for seed scriptingfirebase-emulatorservice on the dockerSteps to test
docker-compose up -d --builddocker-compose exec ts-backend yarn seed:authdocker-compose exec ts-backend yarn migratedocker-compose exec ts-backend yarn seed (this will seed)What should reviewers focus on?
curl -s -X POST ...and test protected API point withs ID tokens and even refresh tokensChecklist