-
Notifications
You must be signed in to change notification settings - Fork 11
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
[feature] Add presets #62
base: main
Are you sure you want to change the base?
Conversation
bfb9d1e
to
29de503
Compare
bba2469
to
e33dcbb
Compare
1b08872
to
88ee986
Compare
Signed-off-by: Devansh Singh <[email protected]>
Create presets service for database operations Signed-off-by: Devansh Singh <[email protected]>
Create endpoints for presets Signed-off-by: Devansh Singh <[email protected]>
Add fixtures to conftest.py for database session and FastAPI TestClient Signed-off-by: Devansh Singh <[email protected]>
Add unittests for /presets endpoint Signed-off-by: Devansh Singh <[email protected]>
Signed-off-by: Devansh Singh <[email protected]>
Add TEUTHOLOGY_API_SQLITE_URI environment variable to tox.ini and unit_tests.yaml Signed-off-by: Devansh Singh <[email protected]>
c8d5b61
to
54099fc
Compare
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.
Looks great, thanks for adding a detailed PR description. It would also be helpful to add the setup instructions (for containerised and containerised) in README.
1c41db5
to
c3506eb
Compare
Added a limit on how many presets a user can store at a time (currently 10) Signed-off-by: Devansh Singh <[email protected]>
c3506eb
to
bd3b11d
Compare
Added a schema for /add endpoint request body for validating cmd attribute. Updated the cmd column datatype from string to JSON for the Presets model. Signed-off-by: Devansh Singh <[email protected]>
f0fa121
to
955c1dc
Compare
Signed-off-by: Devansh Singh <[email protected]>
Signed-off-by: Devansh Singh <[email protected]>
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.
It's working great! Few things I noticed while testing the PR:
-
Username should be case insensitive: if I someone has username JohnDoe, it should return their presets even if username is "johndoe". This is because github usernames are case-insensitive (both vallariag and VallariAg redirect to my profile).
-
/edit: "cmd" should not be required. We can probably send requests to edit without changing cmd. Example, renaming a preset.
-
/edit and /add: Because we have "--suite" inside the
"cmd"
, I don't think we should be asking the user for that input again in request body with"suite"
.
{
"name": "edit_test",
"suite": "string", <- this input can be removed
"cmd": {
"--dry-run": false,
"--non-interactive": false,
"--suite": "string1"
}
}
We can store/update suite
column in our DB based on what is inside "cmd"
's "--suite". And return an error response if suite is missing.
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.
I'm not sure about this:
In /presets/add, I tested with payload:
{
"name": "test_defaults",
"suite": "suite:test",
"cmd": {
"--ceph": "main",
"--ceph-repo": "https://github.com/ceph/ceph-ci.git",
"--machine-type": "testnode",
"--num": "1",
"--priority": "70",
"--suite": "teuthology:no-ceph",
"--suite-branch": "main",
"--suite-repo": "https://github.com/ceph/ceph-ci.git",
"--teuthology-branch": "",
"--verbose": "1",
"<config_yaml>": ["/teuthology/containerized_node.yaml"],
"--owner": "example",
"--sha1": "dda7c8f4d64b3b69a12734212f3b6b3dd1821349"
}
}
But when I read this presets with GET /presets/, I get response:
{
"id": 4,
"name": "test_defaults",
"cmd": {
"dry_run": false,
"non_interactive": false,
"verbose": 1,
"help": false,
"arch": null,
"ceph": "main",
"ceph_repo": "https://github.com/ceph/ceph-ci.git",
"distro": null,
"distro_version": null,
"email": null,
"flavor": "default",
"kernel": "distro",
"machine_type": "testnode",
"newest": "0",
"rerun_status": false,
"rerun_statuses": "fail,dead",
"sha1": "dda7c8f4d64b3b69a12734212f3b6b3dd1821349",
"sleep_before_teardown": "0",
"suite": "teuthology:no-ceph",
"suite_branch": "main",
"suite_dir": null,
"suite_relpath": "qa",
"suite_repo": "https://github.com/ceph/ceph-ci.git",
"teuthology_branch": "",
"validate_sha1": "true",
"wait": false,
"config_yaml": [
"/teuthology/containerized_node.yaml"
],
"owner": "example",
"num": "1",
"priority": "70",
"queue_backend": null,
"rerun": null,
"seed": "-1",
"force_priority": false,
"no_nested_subset": false,
"job_threshold": "500",
"archive_upload": null,
"archive_upload_url": null,
"throttle": null,
"filter": null,
"filter_out": null,
"filter_all": null,
"filter_fragments": "false",
"subset": null,
"timeout": "43200",
"rocketchat": null,
"limit": "0"
},
"suite": "suite:test",
"username": "VallariAg"
}
This added the SuiteArg defaults and saved the presets with that. When user query this preset, they'll probably gonna see all the defaults and get confused by all these extra args.
Which makes me wonder if we don't set these defaults and let teuthology-api call teuthology.suite
with only the args user set, would teuthology be able to run as expected? If we DO need to set defaults before calling teuthology, then we probably need to figure out how to use defaults for teuthology.suite but not save them in DB.
Removed the suite attribute from PresetArgs, created a new model for /update with optional preset fields Signed-off-by: Devansh Singh <[email protected]>
405a537
to
1b9424f
Compare
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 is looking great to me!
Using TestClient
and storing JSON is really nice work :)
@@ -17,3 +17,5 @@ SESSION_SECRET_KEY=my-secret-key | |||
# Path where all logs for teuthology-suite or teuthology-kill would be collected | |||
ARCHIVE_DIR=/archive_dir/ | |||
TEUTHOLOGY_PATH=/teuthology | |||
|
|||
TEUTHOLOGY_API_SQLITE_URI= |
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.
I think we can add here sqlite:////teuthology_api/teuthology.db
as value . The above two paths are also for container setup. What do you think?
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.
I left it because I wasn't sure what the actual path for the database would be. If we're keeping teuthology_api/teuthology.db
as the path, I'll modify the last commit and update the .env.dev
Description
Presets allow users to save teuthology-suite command config with a custom name that can be used again.
This PR includes:
presets
table using SQLModel and setup database migrations using alembic/presets
routeconftest.py
/preset
endpointsFor testing this PR,
TEUTHOLOGY_API_SQLITE_URI
should be added to the.env
file using the formatsqlite:////<path>
start.sh
script inteuthology/docs/docker-compose
alembic upgrade head
command to migrate the databaseWorkflow
The create, update and delete endpoints required a user to be logged in. Users can read any stored preset, but cannot modify or delete them.
Creating a Preset
POST
request to the endpoint/presets/add
with the following request body:?replace=true
to the endpointGet a Preset
GET
request to the endpoint/presets?username=<github_username>&name=<preset_name>
Get All Presets of a User
GET
request to the endpoint/presets/list?username=<github_username>
Update a Preset
PUT
request to the endpoint/presets/edit/<preset_id>
with the parameters that need to updated in the preset in the request bodyDelete a Preset
A
DELETE
request to the endpoint/presets/delete/<preset_id>
Pulpito-ng Integration
Most of the teuthology commands have default parameters, only some such as
suite
andbranch
need to be changed according to the user's need. On the frontend, we can present default values for the command parameters and users can change them accordingly. Once the parameter values are confirmed, they can be sent as a JSON request body to thepresets/add
endpoint.A form can be used for taking the input values, along with a checkbox to set the
replace
query parameter to true, which will delete the user's oldest preset if their count of stored presets is 10.Checklist