-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Remove source/destination spec IDs and replace with source/destinationIds in the backend #528
Conversation
…-images-to-connectordefs
…ove-internal-integrationsjava
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 am very confused with the models naming (doesn't seem to match what we discussed in https://airbytehq.slack.com/archives/C019WEENQRM/p1601672342057100).
|
||
package io.airbyte.commons.docker; | ||
|
||
public class DockerUtil { |
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.
DockerUtils
|
||
final Path path = | ||
Paths.get("../airbyte-server/src/test/resources/json/TestSpecification.json"); | ||
class DockerUtilTest { |
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.
DockerUtilsTest
- sourceId | ||
- specification | ||
- connectionSpecification |
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.
the naming is confusing here and it introduces a new term connection
what about something like: expectedConfigurations
or configurations
@@ -875,16 +875,46 @@ components: | |||
properties: | |||
sourceId: | |||
$ref: "#/components/schemas/SourceId" | |||
SourceCreate: |
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.
Shouldn't that be called SourceDefinitionCreate based on https://airbytehq.slack.com/archives/C019WEENQRM/p1601672342057100
sourceId: | ||
$ref: "#/components/schemas/SourceId" | ||
dockerImageTag: | ||
type: string | ||
SourceRead: |
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.
SourceDefinitionRead
dockerRepository: | ||
type: string | ||
dockerImageVersion: | ||
type: string | ||
SourceReadList: |
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.
SourceDefinitionList
@michel-tricot gonna do the rename in a future PR. I felt the scope of these changes was already decently large and was going to disturb Artem's flow more than needed. That ok with you? |
What
This PR removes spec IDs as first class citizens from everywhere in the (backend) codebase.