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] Remove default namespace toggle #66298

Merged
merged 4 commits into from
Jun 3, 2020

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented May 12, 2020

Summary

Resolves #65666. Removes Use default namespace toggle to match designs and adds validation when namespace field is empty.

image

image

@jen-huang jen-huang added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 Team:Fleet Team label for Observability Data Collection Fleet team labels May 12, 2020
@jen-huang jen-huang requested a review from a team May 12, 2020 19:57
@jen-huang jen-huang self-assigned this May 12, 2020
@elasticmachine
Copy link
Contributor

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

id="xpack.ingestManager.agentConfigForm.namespaceUseDefaultsFieldLabel"
defaultMessage="Use default namespace"
id="xpack.ingestManager.agentConfigForm.namespaceFieldHelpText"
defaultMessage="Leave empty if you do not want a namespace."
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct. There is always a namespace called default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin That's new to me, I thought we allowed no namespaces. So if the user leaves this input empty, we add default to the agent config behind the scenes? Is that the same for the namespace field when creating a data source?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. There must always be a namespace and agent will fill it in if it is not provided. @ph Just to confirm.

I'm still on the fence if we need this setting here at all on the config level. If we do, we must probably better describe what it is for, meaning telling the user this is about where the data is shipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might be something to look at on the agent (and/or EPM?) side. I've used agent configs with no namespaces that end up creating indices without a namespace suffix like metrics-system.core-.

I'll confirm this behavior and create follow up issues where we can discuss how to handle empty namespaces. I think that would be separate from this PR as the PR is UI-only change. The "Leave empty if you do not want a namespace" copy is correct with regards to how we currently handle things, even though it's not the behavior we want.

@jen-huang jen-huang changed the title [Ingest] Remove default namespace toggle [Ingest Manager] Remove default namespace toggle May 13, 2020
@jen-huang jen-huang force-pushed the ingest/fix/default-namespace branch from edef68e to 560bc83 Compare June 2, 2020 00:59
@jen-huang jen-huang requested a review from a team June 2, 2020 01:17
@jen-huang
Copy link
Contributor Author

I've updated this PR to enforce non-empty value for namespace. @elastic/ingest-management Could someone review?

Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

I'm not sure this works as expected.

The UI does not allow me to create a config without a namespace, that's 👍

When I use a new namespace test though, it is not shown in the configuration details (though it does appear in the API response):

image

For comparison, the pre-existing default config shows the default namespace:

image

@jen-huang
Copy link
Contributor Author

@skh Good catch! I think that is unrelated to this PR, I tested on master and found the same thing. I filed #68041 for the bug. It does not affect the default agent config that we create automatically, only user-created ones.

@jen-huang
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@skh skh self-requested a review June 3, 2020 06:13
Copy link
Contributor

@skh skh left a comment

Choose a reason for hiding this comment

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

@jen-huang Thanks for creating an issue!

@jen-huang jen-huang merged commit 168e0e2 into elastic:master Jun 3, 2020
@jen-huang jen-huang deleted the ingest/fix/default-namespace branch June 3, 2020 18:14
jen-huang added a commit to jen-huang/kibana that referenced this pull request Jun 3, 2020
* Remove default namespace toggle

* Fix i18n

* Add namespace validation

Co-authored-by: Elastic Machine <[email protected]>
jen-huang added a commit that referenced this pull request Jun 3, 2020
* Remove default namespace toggle

* Fix i18n

* Add namespace validation

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ingest Manager: default namespace or custom one
5 participants