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

SPE-191 Post migration cleanup #4648

Merged
merged 13 commits into from
Oct 24, 2023
Merged

SPE-191 Post migration cleanup #4648

merged 13 commits into from
Oct 24, 2023

Conversation

couralex6
Copy link
Contributor

Removed:

  • models/*_legacy.sql
  • macros/*_legacy.sql
    • create_seed_as.sql + create_table_as.sql (spark specific)
  • seeds/*_legacy.csv
  • tests/*_legacy.sql

@jeff-dude
Copy link
Member

@couralex6 sorry need to cancel workflow here for a sec :)

there's a large PR running and it's causing failures in other spots. let's give it a few min

@jeff-dude
Copy link
Member

@couralex6 sorry need to cancel workflow here for a sec :)

there's a large PR running and it's causing failures in other spots. let's give it a few min

for reference:
https://github.com/duneanalytics/spellbook/actions/runs/6591243292/job/17909457507?pr=4582

@couralex6
Copy link
Contributor Author

Ready to go @jeff-dude !

@jeff-dude
Copy link
Member

Ready to go @jeff-dude !

okay, let's potentially regroup this afternoon to see if we can merge. i want to prioritize a few other things first

@jeff-dude jeff-dude self-assigned this Oct 23, 2023
@jeff-dude jeff-dude added ready-for-review this PR development is complete, please review dune team created by dune team DuneSQL migration labels Oct 23, 2023
@couralex6 couralex6 changed the title Post migration cleanup SPE-191 Post migration cleanup Oct 23, 2023
@couralex6
Copy link
Contributor Author

couralex6 commented Oct 24, 2023

Also removed:
models/nftearth/optimism/nftearth_optimism_schema.yml
models/oneplanet/polygon/oneplanet_polygon_schema.yml

Moved to _sector/nft/old/platforms/:

  • models/quix/optimism/quix_optimism_schema.yml
  • models/quix/optimism/quix_optimism_sources.yml

Removed old entries from dbt_project.yml

@couralex6
Copy link
Contributor Author

Good to go now! @jeff-dude

Copy link
Member

@jeff-dude jeff-dude left a comment

Choose a reason for hiding this comment

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

approving with couple comments below for future context

Copy link
Member

Choose a reason for hiding this comment

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

double checked these three being removed:

  • oneplanet is good, this PR also removes everything in that directory (all legacy stuff, new stuff moved to _sector/nft/ as expected
  • nftearth is good, same story as above
  • quix has been moved to new directory as well, but the schema / source file remain in old directory models/quix/optimism/ rather than new directory models/_sector/nft/ -- or maybe just dupes?
    • @0xRobin can you double check these schema / source files for quix and clean up in follow-up PR as needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch, I'll move the schema/sources over. 👍

Copy link
Member

Choose a reason for hiding this comment

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

interesting that we had to override these for spark/databricks, but not trino, for s3 location stuff mostly it seems. these macro removals will be good first candidates if anything breaks

@jeff-dude jeff-dude merged commit de76eab into main Oct 24, 2023
@jeff-dude jeff-dude deleted the cleanup branch October 24, 2023 17:54
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dune team created by dune team ready-for-review this PR development is complete, please review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants