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

[Ingest Manager] Data source names should be unique #68019

Closed
mostlyjason opened this issue Jun 2, 2020 · 16 comments · Fixed by #71187
Closed

[Ingest Manager] Data source names should be unique #68019

mostlyjason opened this issue Jun 2, 2020 · 16 comments · Fixed by #71187
Assignees
Labels
bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@mostlyjason
Copy link
Contributor

Each data source should have a unique name in an Agent configuration. However, we can have multiple with matching names.

image

@mostlyjason mostlyjason added bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team Ingest Management:beta1 labels Jun 2, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@ruflin
Copy link
Member

ruflin commented Jun 3, 2020

The data source name is not our unique identifier. This is especially important as the name can change. This is the reason I would not consider this a bug. But having unique data source names could be especially beneficial for our users, especially in case data sources can be reused at one stage.

Should the data source name be unique per config or on a global level?

@jen-huang
Copy link
Contributor

I think data source names should be unique per config, as data source can only be used by one config atm.

@jen-huang jen-huang self-assigned this Jun 3, 2020
@EricDavisX
Copy link
Contributor

From a usability point of view enforcing this can really help users.

Do we want a separate issue about Configuration names too? Or can we handle it here, likely the same arguments apply.

@EricDavisX
Copy link
Contributor

Its a different API that would need to be fixed to enforce this so I will log it separately I guess unless its easier and ok to fix and test them together and I hear back

@ruflin
Copy link
Member

ruflin commented Jun 4, 2020

If we think of a future where we have reusable data source, the name should probably be unique globally? It might become awkward if with RBAC we get into a situation that we tell a user you can't use foo because it is already used but you can't see it. At the same time, that should be fine.

@EricDavisX ++ on separate issue.

@EricDavisX
Copy link
Contributor

logged as here: #68275

@ph ph removed bug Fixes for quality problems that affect the customer experience Ingest Management:beta1 labels Jun 29, 2020
@mostlyjason
Copy link
Contributor Author

@ph this could be a breaking change to implement later. I thought we were trying to get breaking changes in 7.9?

@ph
Copy link
Contributor

ph commented Jul 8, 2020

I have discussed with @ruflin we were not sure we would consider this a bug but more like a feature, this is why we removed the bug label to that issue.

@ph
Copy link
Contributor

ph commented Jul 8, 2020

@paul-tavares @kevinlog Are you using the integration config name as the policy name?

@mostlyjason
Copy link
Contributor Author

mostlyjason commented Jul 8, 2020

It looks like I can have two policies both named "endpoint-1". It seems to work but is confusing from an UX perspective. I'm worried if we launch and users create a bunch of policies that we won't have an easy way to rename them later.

@jen-huang
Copy link
Contributor

this could be a breaking change to implement later. I thought we were trying to get breaking changes in 7.9?

It wouldn't be a breaking change when this is implemented. If a user has the same name for multiple package configs previously, the configs would still remain in place if they do nothing. If they choose to edit one of those configs, we would then surface an error on saving.

BTW it's currently not very easy to get a package config with the same names. By default it is populated with {packageName}-{incrementedNumber}, the user would have to manually edit the name to match an existing one. Also with the change done in #70542, multiple Endpoint package configs are not possible now.

@paul-tavares
Copy link
Contributor

@ph yes, we use the name as the "Policy Name"

@jen-huang
Implementing this in the future would be a breaking change for Endpoint that would require transition. We currently make updates to the Package Configs (still trying to get used to this naming change 😬 ) via API to store some data that is not driven by a UI interaction. So in the case that you detail above (where the error would surface on Update), it would actually break Endpoint.

@ph ph added bug Fixes for quality problems that affect the customer experience Ingest Management:beta1 labels Jul 9, 2020
@ph
Copy link
Contributor

ph commented Jul 9, 2020

Oh, I didn't know you were using a 1:1 match here.

For breaking changes we are in beta so we could do it?

@jen-huang
Copy link
Contributor

@paul-tavares We already implemented functionality for beta to not allow more than one Endpoint package config per agent config, so I don't think we can run into a naming conflict error on update for Endpoint any more?

In any case, I'm working on #70030 right now and plan to squash some related bugs while I'm in that code, so I'll add this bug to my list :) @ph

@jen-huang jen-huang self-assigned this Jul 9, 2020
jen-huang added a commit to jen-huang/kibana that referenced this issue Jul 10, 2020
@paul-tavares
Copy link
Contributor

paul-tavares commented Jul 10, 2020 via email

jen-huang added a commit that referenced this issue Jul 13, 2020
…71187)

* Match add integration page with designs

* Clean up package config layout code

* Match edit integration config page with designs

* Fix typing and i18n issues

* Add back data test subj

* Add loading UI for second step; code clean up

* Fix limited packages incorrect response

* Add ability to create agent config when selecting config to add integration to

* Add error count to input-level panel; memoize children components

* Add error count next to all advanced options toggles

* Move general form error to bottom bar

* #69750 Auto-expand inputs with required & empty (invalid) vars

* #68019 Enforce unique package config names, per agent config

* Fix typing

* Fix i18n

* Fix reloading when new agent config _wasn't_ created

* Memoize edit integration and fix fields not collapsing on edit

* Really fix types
jen-huang added a commit that referenced this issue Jul 13, 2020
…71187) (#71460)

* Match add integration page with designs

* Clean up package config layout code

* Match edit integration config page with designs

* Fix typing and i18n issues

* Add back data test subj

* Add loading UI for second step; code clean up

* Fix limited packages incorrect response

* Add ability to create agent config when selecting config to add integration to

* Add error count to input-level panel; memoize children components

* Add error count next to all advanced options toggles

* Move general form error to bottom bar

* #69750 Auto-expand inputs with required & empty (invalid) vars

* #68019 Enforce unique package config names, per agent config

* Fix typing

* Fix i18n

* Fix reloading when new agent config _wasn't_ created

* Memoize edit integration and fix fields not collapsing on edit

* Really fix types
# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants