Skip to content

Conversation

evanh
Copy link
Member

@evanh evanh commented Nov 2, 2022

It's not clear that migrations belong in configuration. This was already an
issue from the previous work, since the migrations themselves are still part of
the code. There is now a separate effort to figure out where migrations should
live, so reverting these changes in the mean time to avoid having things in a
half finished state.

Before state:

The generic metrics MigrationGroup was defined in configuration, and the groups
code was loading that configuration. The MigrationGroup was a string to allow
for groups being defined outside of Python code.

After state:

All MigrationGroups are defined in the python file, and MigrationGroup has been
changed back to an Enum.

This reverts commit ae89a81

Might cause conflicts with changes from @dbanda @MeredithAnya

It's not clear that migrations belong in configuration. This was already an
issue from the previous work, since the migrations themselves are still part of
the code. There is now a separate effort to figure out where migrations should
live, so reverting these changes in the mean time to avoid having things in a
half finished state.

Before state:

The generic metrics MigrationGroup was defined in configuration, and the groups
code was loading that configuration. The MigrationGroup was a string to allow
for groups being defined outside of Python code.

After state:

All MigrationGroups are defined in the python file, and MigrationGroup has been
changed back to an Enum.

This reverts commit ae89a81
@evanh evanh requested a review from a team as a code owner November 2, 2022 19:33
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2022

Codecov Report

Base: 92.32% // Head: 92.29% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (ef154b1) compared to base (ed5cbef).
Patch coverage: 89.83% of modified lines in pull request are covered.

❗ Current head ef154b1 differs from pull request most recent head 6de9601. Consider uploading reports for the commit 6de9601 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3332      +/-   ##
==========================================
- Coverage   92.32%   92.29%   -0.04%     
==========================================
  Files         701      701              
  Lines       32076    32119      +43     
==========================================
+ Hits        29613    29643      +30     
- Misses       2463     2476      +13     
Impacted Files Coverage Δ
snuba/cli/migrations.py 40.36% <0.00%> (-1.15%) ⬇️
snuba/settings/__init__.py 95.39% <ø> (-0.04%) ⬇️
snuba/migrations/runner.py 92.07% <75.00%> (ø)
snuba/admin/views.py 75.80% <100.00%> (-0.74%) ⬇️
snuba/migrations/groups.py 95.09% <100.00%> (ø)
tests/admin/clickhouse_migrations/test_api.py 100.00% <100.00%> (ø)
tests/migrations/test_groups.py 100.00% <100.00%> (ø)
tests/migrations/test_runner.py 99.33% <100.00%> (ø)
tests/migrations/test_runner_individual.py 91.20% <100.00%> (-2.20%) ⬇️
snuba/settings/settings_distributed.py 0.00% <0.00%> (-100.00%) ⬇️
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@evanh evanh merged commit 9851477 into master Nov 3, 2022
@evanh evanh deleted the evanh/fix/revert-migration-group-configuration branch November 3, 2022 19:04
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.

3 participants