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

Refactor/remove opengraph #699

Merged
merged 8 commits into from
Oct 11, 2022
Merged

Refactor/remove opengraph #699

merged 8 commits into from
Oct 11, 2022

Conversation

jshearer
Copy link
Contributor

@jshearer jshearer commented Sep 27, 2022

Description:

Nuking left-over opengraph stuff after implementing estuary/animated-carnival#58. Original PR was at estuary/animated-carnival#67, which got rolled into the Flow repo.

Deleted scripts/seed_connectors.sql as per @jgraettinger: estuary/animated-carnival#67 (comment)

Blocked on testing locally, which seems to be temporarily broken in current master


This change is Reviewable

alter view live_specs_ext owner to authenticated;

-- Extended view of user draft specifications.
create view draft_specs_ext as
Copy link
Contributor Author

@jshearer jshearer Sep 27, 2022

Choose a reason for hiding this comment

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

@jgraettinger in response to your question about why I'm recreating draft_specs_ext here, it's because we have to recreate live_specs_ext to remove the connector_open_graph field, and draft_specs_ext depends on it. Nothing is changing in draft_specs_ext itself.

@jshearer
Copy link
Contributor Author

@jgraettinger you mentioned wanting me to wrap (something) in begin; commit;. Did you mean the whole migration file? Or just certain lines? I wasn't clear on that

@jshearer jshearer force-pushed the refactor/remove_opengraph branch from 03e1fe6 to d0bd181 Compare September 29, 2022 15:26
@jshearer jshearer marked this pull request as ready for review September 30, 2022 17:42
@jshearer
Copy link
Contributor Author

Alrighty, finally managed to test this successfully locally! Everything seems to work, so I think we're good to merge.

Next steps:

  • Update the ops repo to deploy the new change
  • Then apply the pending migration.
    Since the current agent expects certain columns to exist that will get deleted by the migration, we'll need to roll out the new agent changes before we run the migration.

Copy link
Member

@jgraettinger jgraettinger left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@jshearer jshearer merged commit a56e42a into master Oct 11, 2022
@oliviamiannone oliviamiannone added the docs complete / NA No (more) doc work related to this PR label Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs complete / NA No (more) doc work related to this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants