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

feat: simplify openapi_specifications to data_specifications #64

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

donch1989
Copy link
Member

Because

  • Related to this feat(vdp): remove openapi_specification and add data_specification protobufs#282, currently, we have an openapi_specification field in the connector/operator definition. The original purpose was that we could generate an OpenAPI schema and have some UI to let the user debug on an individual component. However, it turns out that we don't really need that OpenAPI schema, and it makes our response very large and hard to read.

This commit

  • Simplifies openapi_specification into data_specification by preserving only the input and output JSON schema and removing the nested structure of the OpenAPI spec.

Copy link

linear bot commented Mar 6, 2024

@donch1989 donch1989 marked this pull request as draft March 6, 2024 15:21
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 68.18182% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 45.59%. Comparing base (81e34c4) to head (a22d209).

❗ Current head a22d209 differs from pull request most recent head a865862. Consider uploading reports for the commit a865862 to get more accurate results

Files Patch % Lines
pkg/base/component.go 81.25% 3 Missing ⚠️
pkg/base/connector.go 33.33% 2 Missing ⚠️
pkg/base/operator.go 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
- Coverage   45.59%   45.59%   -0.01%     
==========================================
  Files           6        6              
  Lines        1136     1101      -35     
==========================================
- Hits          518      502      -16     
+ Misses        529      511      -18     
+ Partials       89       88       -1     
Flag Coverage Δ
unittests 45.59% <68.18%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -27,9 +27,9 @@ type IConnector interface {
// Add definition
AddConnectorDefinition(def *pipelinePB.ConnectorDefinition) error
// Get the connector definition by definition uid
GetConnectorDefinitionByUID(defUID uuid.UUID, resourceConfig *structpb.Struct, componentConfig *structpb.Struct) (*pipelinePB.ConnectorDefinition, error)
GetConnectorDefinitionByUID(defUID uuid.UUID, resourceConfig *structpb.Struct, component *pipelinePB.ConnectorComponent) (*pipelinePB.ConnectorDefinition, error)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note:
The reason we want to pass component data into the definition is that some fields are dynamically generated. For example, in the Instill Model Connector, the enum inside the definition data JSON schema is generated by the connector setting on the fly. So we need to pass component data into it and generate a correct JSON schema.

The current implementation design looks a little bit weird, but it works. I think we can improve the structure in the future.

Copy link
Collaborator

@jvallesm jvallesm Mar 7, 2024

Choose a reason for hiding this comment

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

👍 Thanks for the clarification, I wondered why we had these unused fields but with this info I realised that each connector implementation (e.g. restapi.Init() must implement this interface and there we make use of these fields.

We'll need to update now the connector and operator repos with the new interface, right? Nevermind, just saw instill-ai/connector#135

@donch1989 donch1989 merged commit 7c27d15 into main Mar 7, 2024
8 checks passed
@donch1989 donch1989 deleted the huitang/ins-3828 branch March 7, 2024 02:35
donch1989 added a commit to instill-ai/connector that referenced this pull request Mar 7, 2024
Because

- Related to instill-ai/component#64, we
simplifies `openapi_specification` into `data_specification` by
preserving only the input and output JSON schema and removing the nested
structure of the OpenAPI spec. We need to adopt this change.

This commit

- Adopts the new GetConnectorDefinition function interface.
Copy link
Collaborator

@jvallesm jvallesm left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -27,9 +27,9 @@ type IConnector interface {
// Add definition
AddConnectorDefinition(def *pipelinePB.ConnectorDefinition) error
// Get the connector definition by definition uid
GetConnectorDefinitionByUID(defUID uuid.UUID, resourceConfig *structpb.Struct, componentConfig *structpb.Struct) (*pipelinePB.ConnectorDefinition, error)
GetConnectorDefinitionByUID(defUID uuid.UUID, resourceConfig *structpb.Struct, component *pipelinePB.ConnectorComponent) (*pipelinePB.ConnectorDefinition, error)
Copy link
Collaborator

@jvallesm jvallesm Mar 7, 2024

Choose a reason for hiding this comment

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

👍 Thanks for the clarification, I wondered why we had these unused fields but with this info I realised that each connector implementation (e.g. restapi.Init() must implement this interface and there we make use of these fields.

We'll need to update now the connector and operator repos with the new interface, right? Nevermind, just saw instill-ai/connector#135

donch1989 pushed a commit that referenced this pull request Mar 12, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.13.0-beta](v0.12.0-beta...v0.13.0-beta)
(2024-03-07)


### Features

* expose description field in component entities
([#62](#62))
([85bbc22](85bbc22))
* simplify `openapi_specifications` to `data_specifications`
([#64](#64))
([7c27d15](7c27d15))


### Bug Fixes

* **vdp:** better casting errors
([#65](#65))
([81e34c4](81e34c4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
namwoam pushed a commit to namwoam/component that referenced this pull request Jun 24, 2024
…till-ai#64)

Because

- Related to this instill-ai/protobufs#282,
currently, we have an `openapi_specification` field in the
connector/operator definition. The original purpose was that we could
generate an OpenAPI schema and have some UI to let the user debug on an
individual component. However, it turns out that we don't really need
that OpenAPI schema, and it makes our response very large and hard to
read.

This commit

- Simplifies `openapi_specification` into `data_specification` by
preserving only the input and output JSON schema and removing the nested
structure of the OpenAPI spec.
namwoam pushed a commit to namwoam/component that referenced this pull request Jun 24, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.13.0-beta](instill-ai/component@v0.12.0-beta...v0.13.0-beta)
(2024-03-07)


### Features

* expose description field in component entities
([instill-ai#62](instill-ai#62))
([85bbc22](instill-ai@85bbc22))
* simplify `openapi_specifications` to `data_specifications`
([instill-ai#64](instill-ai#64))
([7c27d15](instill-ai@7c27d15))


### Bug Fixes

* **vdp:** better casting errors
([instill-ai#65](instill-ai#65))
([81e34c4](instill-ai@81e34c4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
3 participants