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

Fix Alembic migration diversions in dev #2681

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Fix Alembic migration diversions in dev #2681

merged 1 commit into from
Jun 20, 2023

Conversation

e-belfer
Copy link
Member

Looks like we simultaneously pushed two different alembic migration histories into dev and broke the nightly build. Let's fix it!

PR Checklist

  • Merge the most recent version of the branch you are merging into (probably dev).
  • All CI checks are passing. Run tests locally to debug failures
  • Make sure you've included good docstrings.
  • For major data coverage & analysis changes, run data validation tests
  • Include unit tests for new functions and classes.
  • Defensive data quality/sanity checks in analyses & data processing functions.
  • Update the release notes and reference reference the PR and related issues.
  • Do your own explanatory review of the PR to help the reviewer understand what's going on and identify issues preemptively.

@e-belfer e-belfer changed the base branch from main to dev June 19, 2023 13:07
@e-belfer e-belfer requested a review from jdangerx June 19, 2023 13:07
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (926b00c) 87.2% compared to head (72cb01f) 87.2%.

Additional details and impacted files
@@          Coverage Diff          @@
##             dev   #2681   +/-   ##
=====================================
  Coverage   87.2%   87.2%           
=====================================
  Files         87      87           
  Lines      10123   10123           
=====================================
  Hits        8837    8837           
  Misses      1286    1286           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@jdangerx jdangerx left a comment

Choose a reason for hiding this comment

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

Thanks for handling this! It'd be nice to automatically catch these before merge someday :)

@zaneselvans
Copy link
Member

I'm confused about why we don't catch these things. I think the last branch that got merged into dev on Friday was my update to the dependencies of ferc-xbrl-extractor and the tox-pytest action on that PR passed. If there's a problem with the Alembic migration history, why would the CI pass but the nightly build fail? Is the nightly build doing something fundamentally different than the CI with regards to the DB schema?

@e-belfer
Copy link
Member Author

One possibility is that you and I both merged branches that passed in reference to the same dev but yours changed the schema entirely and mine was based on the existing one.

@e-belfer e-belfer merged commit 3dfe15a into dev Jun 20, 2023
10 checks passed
@e-belfer e-belfer deleted the fix-alembic-in-dev branch June 20, 2023 12:05
@jdangerx
Copy link
Member

The nightly build is running alembic upgrade while I think CI doesn't do that. @rousik and I have talked about not running alembic in the nightly build, also - I think it should be pretty easy to do that. Though, if alembic is going to be part of our dev flow, maybe we want alembic to run in CI as opposed to not running it at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants