-
-
Notifications
You must be signed in to change notification settings - Fork 209
Feat: replaced hardcoded test server admin key with env variable #1568
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
Signed-off-by: rohansen856 <rohansen856@gmail.com>
Signed-off-by: rohansen856 <rohansen856@gmail.com>
Signed-off-by: rohansen856 <rohansen856@gmail.com>
fkiraly
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.
This successfully removes the secret from the code base.
My only question, is there a documentation location (e.g., developer documentation) that also needs to get updated about this? Since developers wishing to use the test server now have to:
- obtain credentials (from whom or where?)
- set the environment variable in python
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #1568 +/- ##
===========================================
- Coverage 85.24% 64.38% -20.86%
===========================================
Files 38 36 -2
Lines 5008 4352 -656
===========================================
- Hits 4269 2802 -1467
- Misses 739 1550 +811 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think if any documentation update has to be done it should be in Advanced user guide ( also this is the admin API key for the OpenML test server (https://test.openml.org`) (even though we used very weak credentials for testing purposes: Please do let me know if you have other suggestions on this @fkiraly |
|
@jgyasu, does this need to be added to the developer guide you are writing? |
Metadata
Details
this PR is made to remove the hardcoded test server admin key from the codebase and replace it with environment variable-based authentication.
Summary
openml/config.pyAdded a new environment variable constant for the test server admin key:openml/testing.py. Modified theTestBaseclass to read the admin key from an environment variable instead of using a hardcoded value:Before:
After:
Note:
Noneby default when the environment variable is not setAlso in the tests, Added
pytest.skipifdecorators to tests that require admin privileges in the following test files:tests/test_openml/test_config.pyTest:
test_switch_to_example_configurationAdded decorator:
tests/test_datasets/test_dataset_functions.pyTest:
test_data_statusAdded decorator:
Note:
4. CI Configuration Update (
.github/workflows/test.yml)Added the environment variable to all test execution steps in the GitHub Actions workflow:
Steps updated:
Added to each step:
Impact:
@PGijsbers this requires someone to put the admin key in the github secrets which would be a critical step.