-
Notifications
You must be signed in to change notification settings - Fork 77
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
Check .logfire
for creds to respect 'if-token-present'
setting
#561
Conversation
Deploying logfire-docs with Cloudflare Pages
|
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
…ogfire into check-creds-for-token
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #561 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 133 133
Lines 10238 10256 +18
Branches 1397 1399 +2
=========================================
+ Hits 10238 10256 +18 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
Please can we get this merged and released, I need it for what I'm working on 🙏. |
Sure thing, will finish up today! |
Still have a few follow up questions to resolve with @alexmojaki, then will push across the line. Questions:
|
1 and 3 are OK, it's good to always check the token now because it may be rejected due to exceeded usage. The test is failing because it previously only made requests using the user token |
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
…ogfire into check-creds-for-token
Great, thanks for the feedback! Done! |
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.
Thanks!
Co-authored-by: Alex Hall <alex.mojaki@gmail.com>
Thanks for the feedback :). I think I understand our auth patterns better after working in this space a bit more :) |
Fix #527