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] Do not allow empty namespace #67492

Merged
merged 2 commits into from May 27, 2020

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented May 27, 2020

Summary

Resolve #67473

We should not allow empty namespace as it broke Kibana.

This PR add server side validation to avoid empty namespace, I did not add any client side validation as @jen-huang is currently working on this form #66298

Also done in this PR:

  • I fixed the agent config form to not close, if there is an error.
  • I removed the skipped tests for ingest and added a new one to test agent config endpoint

UI example

Screen Shot 2020-05-27 at 10 37 58 AM

@nchaulet nchaulet added bug Fixes for quality problems that affect the customer experience v8.0.0 v7.8.0 v7.9.0 Team:Fleet Team label for Observability Data Collection Fleet team labels May 27, 2020
@nchaulet nchaulet requested a review from jen-huang May 27, 2020 14:43
@nchaulet nchaulet self-assigned this May 27, 2020
@elasticmachine
Copy link
Contributor

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

@nchaulet nchaulet requested a review from a team May 27, 2020 14:55
@nchaulet nchaulet marked this pull request as ready for review May 27, 2020 14:56
@nchaulet nchaulet added the release_note:skip Skip the PR/issue when compiling release notes label May 27, 2020
@ph ph requested a review from neptunian May 27, 2020 16:09
@paul-tavares
Copy link
Contributor

I think this change will impact the data resources provider we have in Endpoint Functional test (which are currently disabled). I will keep this in mind when re-enabling them.

@elastic/endpoint-management FYI ^

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

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

Code LGTM

@nchaulet nchaulet merged commit 258e7f0 into elastic:master May 27, 2020
@nchaulet nchaulet deleted the feature-validate-config-namespace branch May 27, 2020 17:29
nchaulet added a commit to nchaulet/kibana that referenced this pull request May 27, 2020
nchaulet added a commit to nchaulet/kibana that referenced this pull request May 27, 2020
@@ -9,7 +9,7 @@ import { AgentConfigStatus } from '../../../common';

const AgentConfigBaseSchema = {
name: schema.string(),
namespace: schema.maybe(schema.string()),
namespace: schema.string({ minLength: 1 }),
Copy link
Member

Choose a reason for hiding this comment

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

@nchaulet Can we use this to also do some validation? For example namespace should not contain - as an example.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes we can provide a validate function, where we can add all the custom logic we want

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 release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.8.0 v7.8.1 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ingest manager] Namespace should not accept empty string.
7 participants