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.

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

feat: auditlogs 2 in vfolder #1822

Open
wants to merge 56 commits into
base: main
Choose a base branch
from
Open

feat: auditlogs 2 in vfolder #1822

wants to merge 56 commits into from

Conversation

mirageoasis
Copy link
Collaborator

@mirageoasis mirageoasis commented Jan 5, 2024

revived
#698


📚 Documentation preview 📚: https://sorna--1822.org.readthedocs.build/en/1822/


📚 Documentation preview 📚: https://sorna-ko--1822.org.readthedocs.build/ko/1822/

@github-actions github-actions bot added comp:client Related to Client component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated labels Jan 5, 2024
@github-actions github-actions bot added the size:XL 500~ LoC label Jan 5, 2024
@mirageoasis mirageoasis requested a review from fregataa January 9, 2024 04:51
Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

I will review in detail after we determine to write the audit logs to DB or file system (I personally prefer to write them on DB because it will be hard to track all audit logs under HA setup)

If we decide to use DB for audit logs, then there should be 3 major works.

  1. update alembic migration files
  2. use SQLAlchemy ORM rather than a raw table
  3. change AuditLog graphene �class to graphene relay class. I can do this work if you don't mind!

src/ai/backend/manager/models/audit_logs.py Outdated Show resolved Hide resolved
Comment on lines 351 to 355
try:
return await handler(request)
except Exception:
raise
finally:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
try:
return await handler(request)
except Exception:
raise
finally:
try:
return await handler(request)
finally:

you do not need to catch exception here unless you handle it

Comment on lines 364 to 366
def updated_data(new_data: dict[str, Any]) -> dict[str, Any]:
current_audit_log_data = audit_log_data.get().copy()
return deep_update(current_audit_log_data, new_data)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def updated_data(new_data: dict[str, Any]) -> dict[str, Any]:
current_audit_log_data = audit_log_data.get().copy()
return deep_update(current_audit_log_data, new_data)
def update_audit_log_data(new_data: dict[str, Any]) -> None:
audit_log_data.set(
{**audit_log_data.get(), **new_data}
)

This will work.
And let's not use pydantic v1 API

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe I think using this function is okay
CleanShot 2024-01-10 at 14 40 32@2x

https://docs.pydantic.dev/latest/migration/

this documents says it pydantic.v1.utils import deep_update does not mean it has been deprecated!

@mirageoasis mirageoasis changed the title feature/auditlogs 2 in vfolder feat: auditlogs 2 in vfolder Jan 22, 2024
@achimnol achimnol added this to the 24.03 milestone Jan 26, 2024
@achimnol
Copy link
Member

Remove (unimplemented) created_user_email, created_user_id from sessions and use the session audit logs, which will follow the basic structure of this PR.

@kyujin-cho kyujin-cho mentioned this pull request Mar 29, 2024
@github-actions github-actions bot added area:docs Documentations comp:cli Related to CLI component labels Jun 27, 2024
Copy link
Member

@fregataa fregataa left a comment

Choose a reason for hiding this comment

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

Looks good.
May I update some codes that use SQLAlchemy APIs? We are planning to migrate to SQLAlchemy 2.0 and we should update some of the codes that are not compatible to 2.0 version

Copy link
Member

@achimnol achimnol left a comment

Choose a reason for hiding this comment

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

Remove src/ai/__init__.py and src/ai/backend/__init__.py as these are namespace packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:docs Documentations comp:cli Related to CLI component comp:client Related to Client component comp:manager Related to Manager component require:db-migration Automatically set when alembic migrations are added or updated size:XL 500~ LoC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants