Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[feature] add presets #19

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

[feature] add presets #19

wants to merge 6 commits into from

Conversation

VallariAg
Copy link
Member

@VallariAg VallariAg commented Sep 7, 2023 •
edited

Presets allow users to save teuthology-suite
command config with a unique name for that user
to be reused again.

This PR:

  1. Adds postgres database with alembic
  2. Creates Presets Table
  3. Add endpoints:
    • GET presets/
    • GET presets/list/
    • POST presets/add/
    • UPDATE presets/edit/
    • DELETE presets/delete
  4. Adds a PresetsDatabaseException
  5. Creates PresetsService which is responsible for all presets DB operations

@VallariAg VallariAg marked this pull request as draft September 7, 2023 14:17
@VallariAg VallariAg assigned VallariAg and unassigned kamoltat and amathuria Sep 12, 2023
@VallariAg VallariAg marked this pull request as ready for review September 12, 2023 10:00
@VallariAg VallariAg changed the base branch from main to docker-dev-setup September 25, 2023 19:07
kamoltat
kamoltat reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little comment, also can you please include a more descriptive PR description? Maybe like the purpose of this feature and what it will enable teuthology-api to do

src/routes/presets.py Outdated
detail="You need to be logged in",
headers={"WWW-Authenticate": "Bearer"},
)
db_preset = PresetsService(db).get_by_username_and_name(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment saying checking for existing presets or change the variable name to something db_preset_exists

@VallariAg VallariAg changed the base branch from docker-dev-setup to main October 21, 2023 15:09
@VallariAg
Add 'presets' table with alembic
7f8d4fb 
using postgres database 'tapi_db'

Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg
Add presets routes, schema, model helper functions
0d5b3b2 
creates the following:
models/presets.py: create_preset, get_user_presets, get_preset
routes/presets.py: GET /, GET list/, POST /add
schemas/presets.py: PresetSchema

Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg
Add '/edit' and '/delete' routes
199ee06 
models/presets: add get_preset_id, update_preset, delete_preset
		and PresetsDatabaseException

Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg
Add github auth to /add /edit /delete
38bcaf4 
Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg
gh-actions/start.sh: add tapi_db
ea05df5 
Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg
Add services/presets.py
513e51c 
Signed-off-by: Vallari Agrawal <val.agl002@gmail.com>
@VallariAg VallariAg marked this pull request as draft November 6, 2023 07:53
@VallariAg VallariAg mentioned this pull request May 13, 2024
Open
@VallariAg
Copy link
Member Author

@Devansh3712 I think you can work on top of this branch. There are few things I remember that needed to be addressed since I last updated it:

  1. This PR was opened before t-api's packaging work was done which restructured the whole project - Use PyScaffold to set up a project structure #23. Looks like I rebased it after packaging PR was merged but please see if you're able to get the branch running.
  2. I remember that Zack mentioned that we could use SQLite instead of PostgreSQL - so we can start by changing that if this branch is running without problems.
  3. There's also a merge conflict that we see now so a rebase is probably needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamoltat kamoltat kamoltat left review comments

@amathuria amathuria Awaiting requested review from amathuria

At least 1 approving review is required to merge this pull request.

Assignees

@VallariAg VallariAg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@VallariAg @kamoltat @amathuria