DM-43105: Add migration script for dimension universe 7 #38
Conversation
Pull out the _Context class from the dimension 6 migration to a separate file, since we need all the same stuff for the dimension 7 migration.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 55.82% 55.66% -0.16%
==========================================
Files 37 38 +1
Lines 1297 1315 +18
Branches 281 281
==========================================
+ Hits 724 732 +8
- Misses 536 546 +10
Partials 37 37 ☔ View full report in Codecov by Sentry. |
dhirving
force-pushed
the
tickets/DM-43105
branch
5 times, most recently
from
c6a1030
to
0615730
Compare
|
|
||
| from .butler_attributes import ButlerAttributes | ||
|
|
||
| __all__ = ("MigrationContext",) |
There was a problem hiding this comment.
__all__ should be above all import (except __future__) according to our developers' manual.
| from __future__ import annotations | ||
|
|
||
| import alembic | ||
| import sqlalchemy as sa |
There was a problem hiding this comment.
I think we try not to shorten names to extremely short 2-character abbreviations. There are probably many examples for sa but we should try to avoid using it.
| """Provides access to commonly-needed objects derived from the alembic | ||
| migration context. |
There was a problem hiding this comment.
Maybe add docstrings for attributes?
| self.mig_context = alembic.context.get_context() | ||
| self.schema = self.mig_context.version_table_schema | ||
| bind = self.mig_context.bind | ||
| assert bind is not None, "Can't run offline -- need access to database to migrate data." |
There was a problem hiding this comment.
I think in the future we should try to provide better support for offline migrations. Some scripts already support it, maybe this context class should add some support for it too? Maybe something for the next ticket.
| ctx.attributes.validate_dimensions_json(6) | ||
|
|
||
| _LOG.info("Adding can_see_sky column to exposure table") | ||
| op.add_column(_TABLE, sa.Column(_NEW_COLUMN, sa.Boolean, nullable=True), schema=ctx.schema) |
There was a problem hiding this comment.
Should this and everything below be done with batch migration, just in case we care about sqlite repos?
There was a problem hiding this comment.
Not needed -- all of these operations work and have been tested on sqlite already. They batch isn't needed because there is no alter column, foreign key changes, etc... just adding a column and some update queries.
| # which is closely correlated with whether the sky is visible in the | ||
| # exposure. | ||
| # | ||
| # Any exposure types not in these lists are left null. |
There was a problem hiding this comment.
I had to think (not my usual activity) and parse many lines below to understand what "these lists" mean. Maybe define two lists explicitly?
| ) | ||
|
|
||
|
|
||
| def _find_unhandled_observation_types(ctx: MigrationContext) -> list[str]: |
There was a problem hiding this comment.
docstring would help me here.
dhirving
force-pushed
the
tickets/DM-43105
branch
from
0615730
to
9e38d70
Compare
Checklist