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

Notify the response ops when there is change on connector config #175981

Merged
merged 13 commits into from
Feb 15, 2024

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Jan 31, 2024

Resolves: #175018

This Pr adds an integration test to check the changes on connectorTypes config, secrets and params schemas.
I used validate.schema field as all the connector types have it.

ConnectorTypes has config, secrets and params schemas on validate.schema whereas SubActionConnectorTypes has only config and secrets.

They have multiple params schema as well but only registered and used during action execution.
e.g. https://github.com/ersin-erdal/kibana/blob/main/x-pack/plugins/stack_connectors/server/connector_types/bedrock/bedrock.ts#L57

And here is the explanation why they are not listed in a definition:
https://github.com/ersin-erdal/kibana/blob/main/x-pack/plugins/actions/server/sub_action_framework/validators.ts#L38

We need to do some refactoring to list those schemas on the connector types.

@ersin-erdal ersin-erdal added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.13.0 labels Jan 31, 2024
@ersin-erdal ersin-erdal self-assigned this Jan 31, 2024
@ersin-erdal
Copy link
Contributor Author

/ci

@ersin-erdal ersin-erdal marked this pull request as ready for review February 14, 2024 22:53
@ersin-erdal ersin-erdal requested a review from a team as a code owner February 14, 2024 22:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Verified code, left minor comments 👍

};
});

const connectorTypes: string[] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could you please move this connectorTypes to a mock file?
This will make sure there is only one source of truth and you can add new connectorType to it in the future without worrying to add it in different test files.

});

test('ensure connector types list up to date', () => {
expect(connectorTypes).toEqual(actionTypeRegistry.getAllTypes());
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay, test will fail if new connectorType not added. 👍
Still mock file could be useful if connectorTypes used in multiple test files. Up to you 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added :)

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. Just one question about type. Verified test fails if change is made to connector type schema

schema: {
validate(value: unknown): Type;
validate(value: unknown): T;
getSchema?: () => AnySchema;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this typed as optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it then remembered why i did it so.
1- SubActionsConnectorType converts its definition to ConnectorType def and misses that field (We can fix this I think)
2- There are hundreds of tests cases with ActionType mock without this filed :) And we are using it only in this test, so i made it optional.

@ymao1
Copy link
Contributor

ymao1 commented Feb 15, 2024

We need to do some refactoring to list those schemas on the connector types.

Can we make a followup issue for this?

@ersin-erdal
Copy link
Contributor Author

We need to do some refactoring to list those schemas on the connector types.

Can we make a followup issue for this?

Done: #177025

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ersin-erdal

@ersin-erdal ersin-erdal merged commit 9488c93 into elastic:main Feb 15, 2024
33 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 15, 2024
@ersin-erdal ersin-erdal deleted the 175018-check-connector-changes branch February 15, 2024 17:25
ersin-erdal added a commit that referenced this pull request Feb 15, 2024
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
…stic#175981)

Resolves: elastic#175018

This Pr adds an integration test to check the changes on connectorTypes
config, secrets and params schemas.
I used `validate.schema` field as all the connector types have it.

ConnectorTypes has config, secrets and params schemas on
`validate.schema` whereas SubActionConnectorTypes has only config and
secrets.

They have multiple params schema as well but only registered and used
during action execution.
e.g.
https://github.com/ersin-erdal/kibana/blob/main/x-pack/plugins/stack_connectors/server/connector_types/bedrock/bedrock.ts#L57

And here is the explanation why they are not listed in a definition:

https://github.com/ersin-erdal/kibana/blob/main/x-pack/plugins/actions/server/sub_action_framework/validators.ts#L38

We need to do some refactoring to list those schemas on the connector
types.

---------

Co-authored-by: kibanamachine <[email protected]>
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notify the response ops team whenever a change is made to connector config, secrets or params schema
6 participants