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

[Elastic Connectors] Add index name as input var #11267

Merged
merged 4 commits into from
Oct 7, 2024

Conversation

jedrazb
Copy link
Member

@jedrazb jedrazb commented Sep 27, 2024

Add connector_name as input variable

Update connector package definition to pass connector_name as input variable for the package logic.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

  • Test the final version of this package def e2e (ping @jedrazb for exact steps if needed)

How to test this PR locally

  • Since agentless package is not public yet, ping @jedrazb for steps

Screenshots

Screenshot 2024-10-03 at 17 14 09

@jedrazb jedrazb added the Integration:elastic_connectors Elastic Connectors label Sep 27, 2024
@andrewkroh andrewkroh added the enhancement New feature or request label Oct 3, 2024
@@ -37,11 +37,19 @@ policy_templates:
enabled: false
agentless:
enabled: true
multiple: false
Copy link
Member Author

Choose a reason for hiding this comment

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

Revert to default multiple: true given that now we can have multiple policies with the same input (each input corresponds to a single connector)

Copy link
Member

Choose a reason for hiding this comment

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

Double checking - this would let you set up two github connectors at once? Or this shows you both github and gdrive at once? I'm hoping NOT the later.

Copy link
Member Author

@jedrazb jedrazb Oct 4, 2024

Choose a reason for hiding this comment

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

Actually we might want to keep multiple: false it prevents adding multiple integrations of type elastic connectors to the same policy. I think this is what we want.

Or this shows you both github and gdrive at once?

This doesn't affect this, if you click google drive tile it will just show you "Google Drive Connector" integration. If you click "elastic connectors" (parent package) you will see bothgdrive and github... at some point we need to figure this out. I know it's possible to add custom UI for that parent integration with some level of effort.

The good thing is that the component logic will detect if multiple inputs were passed to the component (e..g when enabling multiple inputs in parent integration tile UI), will log a warning and just accept a single input unit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added multiple: false back in c635362

Copy link
Member

Choose a reason for hiding this comment

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

👍 sounds good

if you click "elastic connectors" (parent package) you will see bothgdrive and github... at some point we need to figure this out.

Proposal to remove that parent tile is being tracked in elastic/package-spec#802

@jedrazb jedrazb marked this pull request as ready for review October 3, 2024 15:14
@jedrazb jedrazb requested a review from a team as a code owner October 3, 2024 15:14
@andrewkroh andrewkroh added the Team:Search-Extract and Transform Search - Extract and Transform [elastic/search-extract-and-transform] label Oct 3, 2024
packages/elastic_connectors/manifest.yml Outdated Show resolved Hide resolved
packages/elastic_connectors/changelog.yml Outdated Show resolved Hide resolved
required: true
show_user: false
description: Connector name to identify the connector in the UI
default: "[Elastic-managed] Github Connector"
Copy link
Member

Choose a reason for hiding this comment

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

Can we add an auto-increment number in here, like we get in the policy and integration name defaults? That might help avoid collisions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately not, we don't have much control over the logic here. Perhaps we could leave this blank and have a custom naming logic (with autoincrement or short randomised string) in connectors python upon creation

Copy link
Member Author

@jedrazb jedrazb Oct 4, 2024

Choose a reason for hiding this comment

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

See, the approach I mentioned in the component PR elastic/connectors#2861 (comment)

I think it would be better to mark this field as optional, don't default to anything and have custom logic to generate the name in connector component. This makes this process independent on fleet logic :) for now I'm appending short id to make connectors unique in frontend, see:

Screenshot 2024-10-04 at 13 33 10

I'm applying changes to the package def

Copy link
Member

Choose a reason for hiding this comment

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

I think that makes sense, to control this in the backend for now. CC @kpollich, in case the idea of auto-increment vars is something worth us filing an issue for, somewhere.

@@ -37,11 +37,19 @@ policy_templates:
enabled: false
agentless:
enabled: true
multiple: false
Copy link
Member

Choose a reason for hiding this comment

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

Double checking - this would let you set up two github connectors at once? Or this shows you both github and gdrive at once? I'm hoping NOT the later.

@jedrazb jedrazb requested a review from seanstory October 4, 2024 11:57
@elasticmachine
Copy link

💚 Build Succeeded

History

@jedrazb jedrazb merged commit 1d9b641 into elastic:main Oct 7, 2024
5 checks passed
@elastic-vault-github-plugin-prod

Package elastic_connectors - 0.0.2 containing this change is available at https://epr.elastic.co/search?package=elastic_connectors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Integration:elastic_connectors Elastic Connectors Team:Search-Extract and Transform Search - Extract and Transform [elastic/search-extract-and-transform]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants