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

add observer cruise id to artifacts table #1337

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Dec 6, 2024

Important

Add observer_cruise_id to artifacts table and update Alembic migration for schema changes.

  • Database Schema:
    • Add observer_cruise_id column to artifacts table in models.py.
    • Update Alembic migration script to add observer_cruise_id to artifacts table and create foreign key to observer_cruises.
    • Modify upgrade() and downgrade() in 2024_12_06_0725-5e266217e9d9_introduce_observercruise_and_.py to handle new column and constraints.

This description was created by Ellipsis for 09a2966. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot 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 to me! Reviewed everything up to 09a2966 in 15 seconds

More details
  • Looked at 81 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. alembic/versions/2024_12_06_0725-5e266217e9d9_introduce_observercruise_and_.py:72
  • Draft comment:
    The downgrade function correctly drops the observer_cruise_id column from the artifacts table, ensuring consistency with the upgrade function.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR adds a new column observer_cruise_id to the artifacts table and updates the Alembic migration script accordingly. However, the downgrade function should also drop the observer_cruise_id column from the artifacts table, which is correctly implemented. The PR also updates the model to include this new column, which is consistent with the migration changes.
2. skyvern/forge/sdk/db/models.py:171
  • Draft comment:
    The addition of observer_cruise_id to the ArtifactModel is consistent with the migration changes, ensuring the ORM model reflects the database schema accurately.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The PR adds a new column observer_cruise_id to the artifacts table in the model, which is consistent with the migration changes. This ensures that the ORM model reflects the database schema accurately.

Workflow ID: wflow_lmMNxY60bDpQuKmj


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@wintonzheng wintonzheng merged commit 95bef7f into main Dec 6, 2024
2 checks passed
@wintonzheng wintonzheng deleted the shu/add_observer_cruise_id branch December 6, 2024 07:28
satansdeer pushed a commit to satansdeer/skyvern that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant