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

[Inference Connector] Changed UI/UX due to the new RFC for the _inference/_service #203363

Merged

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Dec 9, 2024

Related RFC https://docs.google.com/document/d/1DbWpqEKM-MJR2cSJNKLC7RcCNXXQ-_iUShsXKRFKXfk/edit?tab=t.0

Summary

  • removed Task Settings from the UI and schema definition, due to the discussion on Inference sync Dec 5th.
  • renamed provider to service
  • added name and description, use name for the service selector user friendly way
  • dropped options and display type dropdown select, use freeform text input instead
  • dropped display field type, renamed tooltip to the description. Properly updated ConnectorConfigurationFormItems in x-pack/plugins/stack_connectors/public/connector_types/lib/dynamic_config/connector_configuration_form_items.tsx

UI with the updates:
Screenshot 2024-12-08 at 10 09 52 PM

@YulNaumenko YulNaumenko self-assigned this Dec 9, 2024
@YulNaumenko YulNaumenko marked this pull request as ready for review December 9, 2024 06:15
@YulNaumenko YulNaumenko requested review from a team as code owners December 9, 2024 06:15
@YulNaumenko YulNaumenko added v8.18.0 v9.0.0 release_note:skip Skip the PR/issue when compiling release notes backport:version Backport to applied version labels labels Dec 9, 2024
@@ -183,9 +710,6 @@ const openAiConnector = {
model_id: 'gpt-4o',
organization_id: 'test-org',
},
taskTypeConfig: {
user: 'elastic',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@YulNaumenko To get this jest test passing, I had to remove this. Looks like user used to be a task setting for this config but we're not setting it anymore. Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's correct. Thank you for cleaning up the code.

label: 'API Key',
required: true,
sensitive: true,
updatable: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Updatable is in the RFC so I added it to the hard-coded response schema but just set it to true for now since it's not used anywhere in the UI. When I update the API on the Elasticsearch side, I will verify the updatability of each field and set appropriately.

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

LGTM

DROPDOWN = 'dropdown',
CHECKABLE = 'checkable',
}

export interface SelectOption {
label: string;
value: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

is icon needed? I am importing SelectOption from search-connectors and icon is not defined there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, icon is needed to display it in the select if it present

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackConnectors 664.3KB 659.2KB -5.2KB
Unknown metric groups

ESLint disabled line counts

id before after diff
stackConnectors 129 128 -1

Total ESLint disabled count

id before after diff
stackConnectors 135 134 -1

History

cc @YulNaumenko

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM

@YulNaumenko YulNaumenko merged commit 5b10845 into elastic:main Dec 13, 2024
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12318938202

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [Inference Connector] Modified getProvider to use _inference/_services ES API instead of hardcoded values. (#199047)

Manual backport

To create the backport manually run:

node scripts/backport --pr 203363

Questions ?

Please refer to the Backport tool documentation

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 203363 locally

@pgayvallet
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

pgayvallet pushed a commit to pgayvallet/kibana that referenced this pull request Dec 18, 2024
…ence/_service (elastic#203363)

Related RFC
https://docs.google.com/document/d/1DbWpqEKM-MJR2cSJNKLC7RcCNXXQ-_iUShsXKRFKXfk/edit?tab=t.0

## Summary

- removed Task Settings from the UI and schema definition, due to the
discussion on Inference sync Dec 5th.
- renamed provider to service
- added name and description, use name for the service selector user
friendly way
- dropped options and display type dropdown select, use freeform text
input instead
- dropped `display` field type, renamed `tooltip` to the `description`.
Properly updated `ConnectorConfigurationFormItems` in
`x-pack/plugins/stack_connectors/public/connector_types/lib/dynamic_config/connector_configuration_form_items.tsx`

UI with the updates:
<img width="1281" alt="Screenshot 2024-12-08 at 10 09 52 PM"
src="https://github.com/user-attachments/assets/fdb17dd4-c8e4-496b-85e7-03c363546b8e">

---------

Co-authored-by: Ying <[email protected]>
(cherry picked from commit 5b10845)
pgayvallet added a commit that referenced this pull request Dec 18, 2024
…_inference/_service (#203363) (#204690)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Inference Connector] Changed UI/UX due to the new RFC for the
_inference/_service
(#203363)](#203363)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Yuliia
Naumenko","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-13T16:00:59Z","message":"[Inference
Connector] Changed UI/UX due to the new RFC for the _inference/_service
(#203363)\n\nRelated
RFC\r\nhttps://docs.google.com/document/d/1DbWpqEKM-MJR2cSJNKLC7RcCNXXQ-_iUShsXKRFKXfk/edit?tab=t.0\r\n\r\n##
Summary\r\n\r\n- removed Task Settings from the UI and schema
definition, due to the\r\ndiscussion on Inference sync Dec 5th.\r\n-
renamed provider to service\r\n- added name and description, use name
for the service selector user\r\nfriendly way\r\n- dropped options and
display type dropdown select, use freeform text\r\ninput instead\r\n-
dropped `display` field type, renamed `tooltip` to the
`description`.\r\nProperly updated `ConnectorConfigurationFormItems`
in\r\n`x-pack/plugins/stack_connectors/public/connector_types/lib/dynamic_config/connector_configuration_form_items.tsx`\r\n\r\nUI
with the updates:\r\n<img width=\"1281\" alt=\"Screenshot 2024-12-08 at
10 09
52 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/fdb17dd4-c8e4-496b-85e7-03c363546b8e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Ying
<[email protected]>","sha":"5b108453ffe823b5d559952f37da1030a79d3352","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport
missing","v9.0.0","backport:version","v8.18.0"],"number":203363,"url":"https://github.com/elastic/kibana/pull/203363","mergeCommit":{"message":"[Inference
Connector] Changed UI/UX due to the new RFC for the _inference/_service
(#203363)\n\nRelated
RFC\r\nhttps://docs.google.com/document/d/1DbWpqEKM-MJR2cSJNKLC7RcCNXXQ-_iUShsXKRFKXfk/edit?tab=t.0\r\n\r\n##
Summary\r\n\r\n- removed Task Settings from the UI and schema
definition, due to the\r\ndiscussion on Inference sync Dec 5th.\r\n-
renamed provider to service\r\n- added name and description, use name
for the service selector user\r\nfriendly way\r\n- dropped options and
display type dropdown select, use freeform text\r\ninput instead\r\n-
dropped `display` field type, renamed `tooltip` to the
`description`.\r\nProperly updated `ConnectorConfigurationFormItems`
in\r\n`x-pack/plugins/stack_connectors/public/connector_types/lib/dynamic_config/connector_configuration_form_items.tsx`\r\n\r\nUI
with the updates:\r\n<img width=\"1281\" alt=\"Screenshot 2024-12-08 at
10 09
52 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/fdb17dd4-c8e4-496b-85e7-03c363546b8e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Ying
<[email protected]>","sha":"5b108453ffe823b5d559952f37da1030a79d3352"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/203363","number":203363,"mergeCommit":{"message":"[Inference
Connector] Changed UI/UX due to the new RFC for the _inference/_service
(#203363)\n\nRelated
RFC\r\nhttps://docs.google.com/document/d/1DbWpqEKM-MJR2cSJNKLC7RcCNXXQ-_iUShsXKRFKXfk/edit?tab=t.0\r\n\r\n##
Summary\r\n\r\n- removed Task Settings from the UI and schema
definition, due to the\r\ndiscussion on Inference sync Dec 5th.\r\n-
renamed provider to service\r\n- added name and description, use name
for the service selector user\r\nfriendly way\r\n- dropped options and
display type dropdown select, use freeform text\r\ninput instead\r\n-
dropped `display` field type, renamed `tooltip` to the
`description`.\r\nProperly updated `ConnectorConfigurationFormItems`
in\r\n`x-pack/plugins/stack_connectors/public/connector_types/lib/dynamic_config/connector_configuration_form_items.tsx`\r\n\r\nUI
with the updates:\r\n<img width=\"1281\" alt=\"Screenshot 2024-12-08 at
10 09
52 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/fdb17dd4-c8e4-496b-85e7-03c363546b8e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Ying
<[email protected]>","sha":"5b108453ffe823b5d559952f37da1030a79d3352"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Yuliia Naumenko <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants