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.

Sign up for GitHub

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

Jump to bottom

Move 'updateNew' logic into 'update', and remove 'updateNew' #15863

Merged
merged 7 commits into from
Aug 23, 2022
Merged

Move 'updateNew' logic into 'update', and remove 'updateNew' #15863

merged 7 commits into from
Aug 23, 2022

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Aug 22, 2022 •
edited
Loading

What

Resolves #14733

How

This PR moves the logic from the connection updateNew endpoint into update, since only the new endpoint is now being used in production.
This PR also involves replacing usages of updateNew with update and fixing tests.

Recommended reading order

  1. Top to bottom is probably fine

🚨 User Impact 🚨

There should be no user impact resulting from this change

@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server area/frontend Related to the Airbyte webapp labels Aug 22, 2022
@lmossman lmossman temporarily deployed to more-secrets August 22, 2022 22:07 Inactive
@lmossman lmossman temporarily deployed to more-secrets August 22, 2022 23:09 Inactive
@lmossman lmossman force-pushed the lmossman/remove-update-new branch from 317c0f3 to 4204c15 Compare August 23, 2022 00:23
@lmossman lmossman temporarily deployed to more-secrets August 23, 2022 00:26 Inactive
lmossman
lmossman commented Aug 23, 2022
View reviewed changes
airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/BasicAcceptanceTests.java
@@ -934,7 +934,7 @@ void testSyncAfterUpgradeToPerStreamStateWithNoNewData(final TestInfo testInfo)
}

@Test
void testResetAllWhenSchemaIsModified() throws Exception {
void testResetAllWhenSchemaIsModifiedForLegacySource() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to update this test to explicitly use a legacy postgres source that does not support per-stream, since that is the scenario in which all streams are reset as a result of a schema modification

@lmossman lmossman marked this pull request as ready for review August 23, 2022 00:55
@lmossman lmossman requested a review from a team as a code owner August 23, 2022 00:55
@lmossman lmossman requested review from alovew, gosusnp and timroes and removed request for a team August 23, 2022 00:55
alovew
alovew reviewed Aug 23, 2022
View reviewed changes
airbyte-server/src/test/java/io/airbyte/server/handlers/WebBackendConnectionsHandlerTest.java Outdated
assertEquals(connectionRead.getOperationIds(), actualConnectionRead.getOperationIds());
verify(operationsHandler, times(1)).updateOperation(operationUpdate);
}

@Test
void testUpdateConnectionWithOperationsNew() throws JsonValidationException, ConfigNotFoundException, IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we remove 'new' from this test name?

@alovew
Copy link
Contributor

alovew commented Aug 23, 2022

This looks good to me - I think we'll also want to update cloud to remove the updateNew endpoint from ConfigurationApiWrapped.java

krishnaglick
krishnaglick approved these changes Aug 23, 2022
View reviewed changes
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

LGTM for frontend changes

@lmossman
Copy link
Contributor Author

This looks good to me - I think we'll also want to update cloud to remove the updateNew endpoint from ConfigurationApiWrapped.java

Yep, I have a PR for that ready to go here: https://github.com/airbytehq/airbyte-cloud/pull/2441
Just need to update it to bump the OSS dependency once this PR is merged in

@lmossman lmossman temporarily deployed to more-secrets August 23, 2022 16:37 Inactive
@lmossman lmossman merged commit 377a149 into master Aug 23, 2022
@lmossman lmossman deleted the lmossman/remove-update-new branch August 23, 2022 18:36
This was referenced Aug 23, 2022
Closed
Merged
rodireich pushed a commit that referenced this pull request Aug 25, 2022
@lmossman @rodireich
Move 'updateNew' logic into 'update', and remove 'updateNew' (#15863)
bc8a609 
* save

* clean up more usages and remove withRefreshedCatalog

* make webapp use correct endpoint

* add back intercept

* fix acceptance test

* fix log

* remove 'new' from test name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alovew alovew alovew approved these changes

@krishnaglick krishnaglick krishnaglick approved these changes

@gosusnp gosusnp Awaiting requested review from gosusnp

@timroes timroes Awaiting requested review from timroes

Assignees
No one assigned
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/frontend Related to the Airbyte webapp area/platform issues related to the platform area/server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update Connection Endpoint Cleanup
3 participants
@lmossman @alovew @krishnaglick