Feat/clerk middleware user link #74
Conversation
Dao-Ho
left a comment
Dao-Ho
left a comment
There was a problem hiding this comment.
This is an awesome first rip! 🔥 🔥 I left some comments for improvements. Also, for feats like these, it's very important to document your updates to the dev environment (include them in the pr body) and also attach a visual clip of the full flow working.
| DOCKER_IMAGE_NAME := selfserve-backend | ||
| PORT := 8080 | ||
| ENV_FILE := config/.env | ||
| NGROK_DOMAIN := undetestably-unidentical-selah.ngrok-free.dev |
There was a problem hiding this comment.
This ngrok domain is exclusively yours, we don't want this hardcoded. It also shouldn't be public, let's move it to be pulled from .env
I'd also look into regenerating your domain since you generally don't want your free plan's domain public like this
| go build -o bin/$(APP_NAME) $(CMD_PATH) | ||
|
|
||
| run: db-start | ||
| @ngrok http --url=$(NGROK_DOMAIN) $(PORT) & \ |
There was a problem hiding this comment.
I wouldn't call the tunnel in the make run script. A couple reasons here:
- It's not always needed, adding extra overhead unnecessarily
- More layers of errors that the script needs to handle, the caller should determine if they need to tunnel something
- other services may be calling this script. Make run should just be doing exactly what it states to be doing- running our services/server, not forwarding our port in addition (this is a devs only step). Ex. Consider our CI pipeline trying to call make run
|
|
||
| func setupClerk() { | ||
| if os.Getenv("ENV") == "development" { | ||
| clerksdk.SetKey(os.Getenv("DEV_CLERK_SECRET_KEY")) |
There was a problem hiding this comment.
new vars added to .env should also be added to .env.sample
Also if you're adding new things like secrets, I'd make sure to add that to the PR body, it's an important change that affects the dev environment.
| usersHandler := handler.NewUsersHandler(usersRepo) | ||
| reqsHandler := handler.NewRequestsHandler(repository.NewRequestsRepo(repo.DB)) | ||
| hotelsHandler := handler.NewHotelsHandler(repository.NewHotelsRepo(repo.DB)) | ||
| whVerifier, err := handler.NewWebhookVerifier() |
There was a problem hiding this comment.
whVerifier is really vague. Something I like doing is trying to use your variables that you name in a sentence and see if it makes sense. Let's rename this to be more explicit.
| hotelsHandler := handler.NewHotelsHandler(repository.NewHotelsRepo(repo.DB)) | ||
| whVerifier, err := handler.NewWebhookVerifier() | ||
| if err != nil { | ||
| fmt.Print(err) |
There was a problem hiding this comment.
why fmt.Print here? Let's throw the appropriate error
| return svix.NewWebhook(os.Getenv("DEV_CLERK_WEBHOOK_SIGNATURE")) | ||
| } | ||
|
|
||
| func NewClerkHandler(userRepo storage.UsersRepository, WebhookVerifier WebhookVerifier) *ClerkHandler { |
There was a problem hiding this comment.
Your naming convention has been ClerkWebHook_. Let's maintain that here as well for consistency. NewClerkWebHookHandler
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
making this a util was :chef-kiss: 🐐
| err := r.db.QueryRow(ctx, ` | ||
| INSERT INTO public.users ( | ||
| first_name, last_name, employee_id, profile_picture, role, department, timezone | ||
| ) VALUES ( |
There was a problem hiding this comment.
You're missing clerk_id in your insert statement. Was the insert working without?
Description
make runcommand to make the backend visible to clerkType of Change
Related Issue(s)
Backend part of #71
What Changed?
Testing & Validation
How this was tested
Unfinished Work & Known Issues
Missing good environment key selection, need the prod url and start a prod instance on clerk
None, this PR is complete and production-ready
The following items are intentionally deferred:
Notes & Nuances
Pre-Merge Checklist
Code Quality
Testing & CI
Documentation
Reviewer Notes
I did not add API docs to the webhook endpoint because this should never be called by our frontends