Add _connector API #2367

jedrazb · 2023-12-12T10:51:12Z

👋 Hey, I'm not sure if it's a best practice to add the client types after the FF. If I mislabelled it please let me know. The connectors API actions are introduced in 8.12.

Changes

Copy endpoints from es rest-api-spec
Copy and adapt types from Kibana ts definition
Added types for
- connector.put
- connector.post
- connector.delete
- connector.get
- connector.list
- connector.check_in
- connector.update_error
- connector.update_scheduling
- connector.update_filtering
- connector.update_configuration
- connector.update_pipeline
- connector.last_sync <- it should be update_last_sync, we should followup to fix this in 8.13 on ES side

output/schema/validation-errors.json

…e-update-delete-list-get

Anaethelion

Left a few comments.

I've also taken the liberty to add the links to the documentation within the endpoints, feel free to edit/update if I made any mistake.

specification/connector/_types/Connector.ts

Co-authored-by: Laurent Saint-Félix <[email protected]>

…e-update-delete-list-get

jedrazb · 2024-01-02T16:27:42Z

Hey @Anaethelion thank you for review and adding the documentation links 👍 . I addressed your feedback and also updated the json spec files with correct doc links.

Given the release timeline is it still ok to keep the backport 8.12 label?

There are couple of more endpoints to be added (related to _connector/_sync_job API) in a subsequent PR.

jedrazb · 2024-01-04T11:14:01Z

Hey @swallez 👋 since Anaethelion is OOO this week, I was suggested to add you as a reviewer to this PR. Could you take a look?

swallez

Thanks for this big PR! I left a number of comments related to the style and constraints expected by the API specification.

Some comments only appear once but have many occurrences, so let me restate them here:

string | number | boolean | null should be replaced by ScalarValue, or FieldValue if it actually also accepts objects
the use of ' | null' should be avoided unless null has a special meaning. Use optional fields instead.

specification/connector/_types/Connector.ts

swallez · 2024-01-05T16:15:24Z

specification/connector/_types/Connector.ts

+}
+
+enum DisplayType {
+  TEXTBOX = 'textbox',


For enums you don't need to specify both identifier and value if they are the same (even with a different casing). Language generators will take care of using their casing convention. So this can be

enum DisplayType { textbox, textarea, numeric...

Thank you for pointing this out 👍 I'm guilty of just copy-pasting most of the types from defs in Kibana

specification/connector/_types/Connector.ts

swallez · 2024-01-05T17:45:22Z

specification/connector/_types/Connector.ts

+  display: DisplayType
+  label: string
+  options: SelectOption[]
+  order?: number | null


Is the value null different from missing (i.e. optional) field? If not, remove null (many occurrences).

I revised the types and I think for majority of cases we can safely mark fields as optional (instead of having them nullable).

The are some exceptions like: error fields that are (string | null), I can think of some situations where we want to mark error as null (when connector gets back to a healthy state).

Also, last_sync... fields can have explicit null value in index. Could marking them optional instead of nullable break things, given that we use Connector type to describe return type of get/list endpoints?

Ok. So we have to implement #2049 that clearly distinguishes "null as optional" and "null as an actual value" in the spec. Adding that on my immediate todo list.

Does it imply that this PR needs to wait until #2049 is implemented? We are not in a rush, just want to clarify

Yes, because currently null is considered as the equivalent of "missing" in languages where objects are represented as data structures and not dictionaries (i.e. all strictly typed languages). #2049 is meant to capture "null is an actual value" in the API spec so that we can generate code that correctly handles this.

Hey @swallez, just a gentle ping regarding #2049. Do you have a timeline when we can expect this to be available?

specification/connector/_types/Connector.ts

… type, add CustomSchedulingConfigurationOverrides type

…e-update-delete-list-get

jedrazb · 2024-01-09T09:28:02Z

Hey @Anaethelion @swallez I hope that all feedback is addressed now, can you take a look at this PR again?
Edit, I just realised you left some comments, taking a look 👀

jedrazb · 2024-01-09T13:20:39Z

CI steps are breaking in the setup job phase with

Error: Internal server error occurred while resolving "actions/checkout@v[2](https://github.com/elastic/elasticsearch-specification/actions/runs/7461636399/job/20302186145?pr=2367#step:1:2)"

Ahh probably it's github issue, see https://www.githubstatus.com/ - rerunning manually fixed it

…e-update-delete-list-get

swallez

Looking good! There are a few minor naming issues left, and we'll be good to go!

swallez · 2024-04-10T14:55:58Z

specification/connector/list/ConnectorListRequest.ts

+    /**
+     * A comma-separated list of connector index names to fetch connector documents for
+     */
+    index_name?: Fields


Fields is meant to hold a list of index field names. If these are ES indices, then Indices should be used.

swallez · 2024-04-10T14:56:48Z

specification/connector/list/ConnectorListRequest.ts

+    /**
+     * A comma-separated list of connector names to fetch connector documents for
+     */
+    connector_name?: Fields


Use Names instead

swallez · 2024-04-10T14:57:20Z

specification/connector/list/ConnectorListRequest.ts

+    /**
+     * A comma-separated list of connector service types to fetch connector documents for
+     */
+    service_type?: Fields


Are service type identifiers? Then use Ids, or otherwise Names.

They are arbitrary strings, so it is Names for our case

swallez · 2024-04-10T16:09:44Z

specification/connector/_types/Connector.ts

+  configuration: ConnectorConfiguration
+  custom_scheduling: ConnectorCustomScheduling
+  description?: string
+  error: WithNullValue<string>


Is this field really required? Will it always be present? If yes, this means that null will mean "meaningful null" in responses. What does this mean?

Same question for a few other uses of required WithNullValue fields in this PR.

Yes, I do agree with you here. Since the Connector denotes a response type. In a response, a missing (optional) field does indicate that the field is in fact null.

I will adapt this, but I the meaningful is needed for requests definitions to set certain fields to null.

pquentin · 2024-04-10T17:51:19Z

"Validate APIs / build" was failing because of me, I fixed it and retried the build.

github-actions · 2024-04-11T07:58:57Z

Following you can find the validation results for the APIs you have changed.

API	Status	Request	Response
`connector.check_in`	⚪	Missing test	Missing test
`connector.delete`	⚪	Missing test	Missing test
`connector.get`	⚪	Missing test	Missing test
`connector.last_sync`	⚪	Missing test	Missing test
`connector.list`	⚪	Missing test	Missing test
`connector.post`	⚪	Missing test	Missing test
`connector.put`	⚪	Missing test	Missing test
`connector.update_api_key_id`	⚪	Missing test	Missing test
`connector.update_configuration`	⚪	Missing test	Missing test
`connector.update_error`	⚪	Missing test	Missing test
`connector.update_filtering`	⚪	Missing test	Missing test
`connector.update_index_name`	⚪	Missing test	Missing test
`connector.update_name`	⚪	Missing test	Missing test
`connector.update_native`	⚪	Missing test	Missing test
`connector.update_pipeline`	⚪	Missing test	Missing test
`connector.update_scheduling`	⚪	Missing test	Missing test
`connector.update_service_type`	⚪	Missing test	Missing test
`connector.update_status`	⚪	Missing test	Missing test

You can validate these APIs yourself by using the make validate target.

swallez

LGTM! 🎉

flobernd · 2024-04-11T17:25:40Z

specification/connector/update_index_name/ConnectorUpdateIndexNameRequest.ts

+   * The connector index name
+   */
+  body: {
+    index_name: WithNullValue<string>


@swallez @Anaethelion I'm late to the party, but shouldn't we use the IndexName alias instead of string at these places?

Like:

index_name: WithNullValue<IndexName>

jedrazb added specification backport 8.12 labels Dec 12, 2023

jedrazb requested a review from a team as a code owner December 12, 2023 10:51

jedrazb changed the title ~~[Connector API] Add types for create, list, get, delete endpoints~~ [Connector API] Add types for Connector API endpoints Dec 13, 2023

jedrazb changed the title ~~[Connector API] Add types for Connector API endpoints~~ [Connector API] Add types for Connector API actions Dec 13, 2023

jedrazb changed the title ~~[Connector API] Add types for Connector API actions~~ [Connector API] Add _connector API Dec 13, 2023

Add _connector API definition

ba8f107

jedrazb force-pushed the connectors-api-add-connectors-api-spec-create-update-delete-list-get branch from 8e44f4e to ba8f107 Compare December 13, 2023 13:30

jedrazb commented Dec 13, 2023

View reviewed changes

output/schema/validation-errors.json Show resolved Hide resolved

Merge branch 'main' into connectors-api-add-connectors-api-spec-creat…

e802bf9

…e-update-delete-list-get

jedrazb changed the title ~~[Connector API] Add _connector API~~ Add _connector API Dec 13, 2023

Add documentation links to endpoints

033e1f5

Anaethelion requested changes Dec 21, 2023

View reviewed changes

jedrazb added backport 8.12 and removed backport 8.12 labels Jan 2, 2024

jedrazb and others added 5 commits January 2, 2024 16:09

Update specification/connector/_types/Connector.ts

9bb4065

Co-authored-by: Laurent Saint-Félix <[email protected]>

Update specification/connector/_types/Connector.ts

6e2dcf6

Co-authored-by: Laurent Saint-Félix <[email protected]>

Update specification/connector/_types/Connector.ts

9bd62fb

Co-authored-by: Laurent Saint-Félix <[email protected]>

Update json_spec with correct doc links, address review feedback

cb6fb94

Merge branch 'main' into connectors-api-add-connectors-api-spec-creat…

2aa05bd

…e-update-delete-list-get

jedrazb requested review from Anaethelion and swallez January 2, 2024 16:27

Update typo in the connector.last_sync comment

0de50c4

swallez requested changes Jan 5, 2024

View reviewed changes

jedrazb added 2 commits January 8, 2024 12:27

Address review feedback, use optional where possible, use ScalarValue…

9413107

… type, add CustomSchedulingConfigurationOverrides type

Merge branch 'main' into connectors-api-add-connectors-api-spec-creat…

65c8a8d

…e-update-delete-list-get

jedrazb requested a review from swallez January 8, 2024 11:28

jedrazb added 2 commits January 9, 2024 14:15

Use long/integer/double types, use @Variants for Validation def

6eed5ce

Re-run make contrib

a3376c9

jedrazb removed the backport 8.12 label Jan 31, 2024

swallez mentioned this pull request Apr 3, 2024

Add null value and Setting type #2478

Merged

jedrazb added 8 commits April 9, 2024 12:17

Merge branch 'main' into connectors-api-add-connectors-api-spec-creat…

e978bd6

…e-update-delete-list-get

Update _json_spec to current ES version

7b781fa

Merge branch 'main' into connectors-api-add-connectors-api-spec-creat…

f4d42e2

…e-update-delete-list-get

Merge branch 'main' into connectors-api-add-connectors-api-spec-creat…

ce9338e

…e-update-delete-list-get

Update API definitions

7834fdd

Update connector list params

4cade96

Add missing param for list request

30271e3

Fix typo in spec_utils

04b8b13

swallez requested changes Apr 10, 2024

View reviewed changes

swallez reviewed Apr 10, 2024

View reviewed changes

Address review feedback

f7deb4a

jedrazb requested a review from swallez April 11, 2024 08:02

swallez approved these changes Apr 11, 2024

View reviewed changes

jedrazb enabled auto-merge (squash) April 11, 2024 09:41

Anaethelion approved these changes Apr 11, 2024

View reviewed changes

jedrazb merged commit c1ed27e into main Apr 11, 2024
6 checks passed

jedrazb deleted the connectors-api-add-connectors-api-spec-create-update-delete-list-get branch April 11, 2024 11:01

flobernd reviewed Apr 11, 2024

View reviewed changes

This was referenced Apr 17, 2024

Connector SyncJob API spec #2503

Merged

[Connector API] Add missing 8.14 changes #2527

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add _connector API #2367

Add _connector API #2367

jedrazb commented Dec 12, 2023 •

edited

Loading

Anaethelion left a comment

jedrazb commented Jan 2, 2024 •

edited

Loading

jedrazb commented Jan 4, 2024

swallez left a comment

swallez Jan 5, 2024

jedrazb Jan 8, 2024

swallez Jan 5, 2024

jedrazb Jan 8, 2024 •

edited

Loading

swallez Jan 8, 2024

jedrazb Jan 9, 2024

swallez Jan 9, 2024

jedrazb Jan 31, 2024 •

edited

Loading

jedrazb commented Jan 9, 2024 •

edited

Loading

jedrazb commented Jan 9, 2024 •

edited

Loading

swallez left a comment

swallez Apr 10, 2024

swallez Apr 10, 2024

swallez Apr 10, 2024

jedrazb Apr 11, 2024

swallez Apr 10, 2024

jedrazb Apr 11, 2024

pquentin commented Apr 10, 2024

github-actions bot commented Apr 11, 2024

swallez left a comment

flobernd Apr 11, 2024

Add _connector API #2367

Add _connector API #2367

Conversation

jedrazb commented Dec 12, 2023 • edited Loading

Changes

Anaethelion left a comment

Choose a reason for hiding this comment

jedrazb commented Jan 2, 2024 • edited Loading

jedrazb commented Jan 4, 2024

swallez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedrazb Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedrazb Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

jedrazb commented Jan 9, 2024 • edited Loading

jedrazb commented Jan 9, 2024 • edited Loading

swallez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pquentin commented Apr 10, 2024

github-actions bot commented Apr 11, 2024

swallez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedrazb commented Dec 12, 2023 •

edited

Loading

jedrazb commented Jan 2, 2024 •

edited

Loading

jedrazb Jan 8, 2024 •

edited

Loading

jedrazb Jan 31, 2024 •

edited

Loading

jedrazb commented Jan 9, 2024 •

edited

Loading

jedrazb commented Jan 9, 2024 •

edited

Loading