-
Notifications
You must be signed in to change notification settings - Fork 173
fix: use temp directory in creds tests to prevent auth.json pollution #3311
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
Conversation
Fixes knative#3158 When HOME is empty during tests, testConfigPath now falls back to t.TempDir() instead of returning an empty string. This prevents auth.json from being created in the pkg/creds/ directory.
|
Welcome @Kunal1522! It looks like this is your first PR to knative/func 🎉 |
|
Hi @Kunal1522. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
|
Hi @Kunal1522 does this also fix the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3311 +/- ##
==========================================
- Coverage 55.09% 55.05% -0.05%
==========================================
Files 170 170
Lines 19851 19851
==========================================
- Hits 10937 10929 -8
- Misses 7845 7853 +8
Partials 1069 1069
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
however i am not sure why E2E - Runtimes (python) fails ?looks unrelated to my changes |
that mightve been a flake, I wasnt able to see them because of new changes. I reran the tests again |
gauron99
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.
/lgtm
Thanks for the contribution!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauron99, Kunal1522 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
|
One more thing @Kunal1522 . Could you please remove the TODO in Sorry to approve and then ask for changes, i remembered too late we have that in gitignore still Feel free to ping me again so i can reapprove |
Since the test now uses a temp directory, auth.json is no longer created in pkg/creds/, so we don't need to ignore it anymore.
|
Done @gauron99 Ready for re-review when you get a chance. Thanks! |
|
thanks! /lgtm |

Fixes #3158
When HOME is empty during tests, testConfigPath now falls back to t.TempDir() instead of returning an empty string. This prevents auth.json from being created in the pkg/creds/ directory.