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

Set breaking change to false when connection is fixed #20315

Merged
merged 12 commits into from
Dec 14, 2022

Conversation

alovew
Copy link
Contributor

@alovew alovew commented Dec 9, 2022

When users update their connection's sync catalog after there is a breaking schema change, the breaking_change field should be set to false

@alovew alovew requested a review from a team as a code owner December 9, 2022 18:44
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/server labels Dec 9, 2022
@alovew alovew temporarily deployed to more-secrets December 9, 2022 18:45 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 9, 2022 18:45 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 9, 2022 18:58 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 9, 2022 18:58 — with GitHub Actions Inactive
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@alovew alovew temporarily deployed to more-secrets December 9, 2022 19:13 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 9, 2022 19:13 — with GitHub Actions Inactive
@@ -654,6 +654,11 @@ protected static ConnectionUpdate toConnectionPatch(final WebBackendConnectionUp
final List<UUID> finalOperationIds) {
final ConnectionUpdate connectionPatch = new ConnectionUpdate();

if (webBackendConnectionPatch.getSyncCatalog() != null) {
// the user has updated their sync catalog, so there is no breaking change
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check that all the change are now not breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was considered in the tech design but it requires getting the source schema and calculating a diff again. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a simpler way of doing it based on FE validations (they can't update an invalid sync catalog), but maybe it's better for us to re-validate here too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just realized the env variable won't work if we do this, since we're marking connections as broken even in the absence of the env variable. so I'll have to update to fetch the most recent actor catalog and do a diff to check whether it's broken

@octavia-squidington-iv octavia-squidington-iv removed area/server area/platform issues related to the platform labels Dec 10, 2022
@alovew alovew temporarily deployed to more-secrets December 10, 2022 00:13 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 10, 2022 00:14 — with GitHub Actions Inactive
@alovew alovew force-pushed the anne/update-endpoing-fixes-broken-connection branch from 3feaca1 to d45e124 Compare December 12, 2022 18:46
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/server labels Dec 12, 2022
@alovew alovew temporarily deployed to more-secrets December 12, 2022 18:48 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 18:48 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 19:12 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 19:13 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 20:20 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 20:20 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 21:16 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 21:17 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 21:57 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 12, 2022 21:57 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 13, 2022 21:44 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets December 13, 2022 21:44 — with GitHub Actions Inactive
@alovew alovew merged commit 3dae1f1 into master Dec 14, 2022
@alovew alovew deleted the anne/update-endpoing-fixes-broken-connection branch December 14, 2022 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants