-
-
Notifications
You must be signed in to change notification settings - Fork 989
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
core: Tidy contributor onboarding, fix typos. #12700
base: main
Are you sure you want to change the base?
core: Tidy contributor onboarding, fix typos. #12700
Conversation
✅ Deploy Preview for authentik-storybook canceled.
|
✅ Deploy Preview for authentik-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12700 +/- ##
=======================================
Coverage 92.75% 92.76%
=======================================
Files 769 769
Lines 38840 38898 +58
=======================================
+ Hits 36026 36083 +57
- Misses 2814 2815 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
coverage run manage.py test --keepdb authentik | ||
coverage html | ||
coverage report | ||
poetry run coverage run manage.py test --keepdb authentik |
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.
We usually assume devs will already be in a poetry shell
, but I don't think this should matter anyway.
However, in CI (.github/workflows/ci-main.yml
for instance), we do poetry run make test
, so that should be updated as 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.
Hi @rissson!
Thanks for reviewing my PR. I'm open to either approach, but it may help to keep the prefix.
When running make test
without a Poetry shell activated, make
doesn't know where to find the coverage
command and emits an error without a hint at the solution.
❯ make test
coverage run manage.py test --keepdb authentik
make: coverage: No such file or directory
make: *** [test] Error 1
The virtual environment instructions appear in the full-stack setup docs, but my concern is that contributors who start with the front-end or docs environments might miss the Python requirements when switching contexts.
Since ci-main.yml uses the poetry prefix consistently, would it make sense to keep it for now? We could look at removing it from both places as a separate task if you think that would be valuable.
Thanks for your feedback!
fe330fd
to
10893ee
Compare
Details
This PR contains a subset of fixes discovered when progressing through the authentik contributor guide. Highlights include:
Checklist
ak test authentik/
)make lint-fix
)If an API change has been made
make gen-build
)If changes to the frontend have been made
make web
)If applicable
make website
)