-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
update open api in correct place #14652
Conversation
alovew
commented
Jul 12, 2022
•
edited
Loading
edited
- Update API in correct place to endpoint changes
- Add in skipReset functionality to new endpoint
final List<StreamDescriptor> apiStreamsToReset = getStreamsToReset(catalogDiff); | ||
List<io.airbyte.protocol.models.StreamDescriptor> streamsToReset = | ||
apiStreamsToReset.stream().map(ProtocolConverters::streamDescriptorToProtocol).toList(); | ||
|
||
ConnectionRead connectionRead; | ||
connectionRead = connectionsHandler.updateConnection(connectionUpdate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In its current state, this PR will reintroduce the bug that Benoit called out, because this is now saving the updated connection before we calculate the diff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could add a verify InOrder to the tests to catch this in our unit tests, to prevent anyone from reintroducing this bug in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the fetch of the existing catalog before the save though, so I thought that would take care of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I missed that, my bad. This should be fine then 👍
if (stateType == ConnectionStateType.LEGACY || stateType == ConnectionStateType.NOT_SET || stateType == ConnectionStateType.GLOBAL) { | ||
streamsToReset = configRepository.getAllStreamsForConnection(connectionId); | ||
final List<StreamDescriptor> apiStreamsToReset = getStreamsToReset(catalogDiff); | ||
List<io.airbyte.protocol.models.StreamDescriptor> streamsToReset = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need Benoit's configuration diff change from here for this to be fully working as desired: https://github.com/airbytehq/airbyte/pull/14626/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok if these go in separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think it's okay, I just wanted to call out that if someone tries to use this endpoint before Benoits change is in then changes to only the configuration of streams will not result in a reset
@@ -1975,6 +1975,27 @@ paths: | |||
$ref: "#/components/schemas/WebBackendConnectionRead" | |||
"422": | |||
$ref: "#/components/responses/InvalidInputResponse" | |||
/v1/web_backend/connections/updateNew: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is the superseeded version of an already existing API, could we just name it /v2/web_backend/connections/update
? Since we have a version number already in our API paths, this feels like the ideal use-case to leverage it :)