Skip to content

[Ingest Manager] Guard against empty stream.datasource and namespace#18769

Merged
michalpristas merged 5 commits intoelastic:masterfrom
michalpristas:agent-fix-namespace
May 27, 2020
Merged

[Ingest Manager] Guard against empty stream.datasource and namespace#18769
michalpristas merged 5 commits intoelastic:masterfrom
michalpristas:agent-fix-namespace

Conversation

@michalpristas
Copy link
Copy Markdown
Contributor

@michalpristas michalpristas commented May 27, 2020 •
edited
Loading

What does this PR do?

This PR fallbacks to default values if namespace or datasource is an empty string

Why is it important?

Empty string is not a valid value

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@michalpristas michalpristas added bug review needs_backport PR is waiting to be backported to other branches. Team:Ingest Management Ingest Management:alpha1 Group issues for ingest management alpha1 labels May 27, 2020
@michalpristas michalpristas self-assigned this May 27, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 27, 2020

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

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels May 27, 2020
@michalpristas michalpristas changed the title [Ingest Manager] Guard agains empty stream.datasource and namespace [Ingest Manager] Guard against empty stream.datasource and namespace May 27, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 27, 2020 •
edited
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #18769 updated]

  • Start Time: 2020-05-27T15:13:41.593+0000

  • Duration: 31 min 9 sec

Test stats 🧪

Test Results
Failed 0
Passed 504
Skipped 127
Total 631

ruflin
ruflin reviewed May 27, 2020
View reviewed changes
x-pack/elastic-agent/pkg/agent/transpiler/rules_test.go
dataset: dsds
index: mytype-dsds-nsns
- name: All specified with empty strings
namespace: ""
Copy link
Copy Markdown
Contributor

@ruflin ruflin May 27, 2020

Choose a reason for hiding this comment

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

Shouldn't the expected now be default?

Copy link
Copy Markdown
Contributor Author

@michalpristas michalpristas May 27, 2020

Choose a reason for hiding this comment

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

this is test for injecting index, it does not change existing values.
we either generate index based on these values or we're adding add_fields processor with correct values

Copy link
Copy Markdown
Contributor

@ruflin ruflin May 27, 2020

Choose a reason for hiding this comment

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

we need both. values and index must match.

In a follow up we can discuss, if we should even error if it is "".

Copy link
Copy Markdown
Contributor

@ph ph May 27, 2020

Choose a reason for hiding this comment

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

@michalpristas I also thought the result should be default?

Copy link
Copy Markdown
Contributor Author

@michalpristas michalpristas May 27, 2020

Choose a reason for hiding this comment

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

i think we can fix it but outside of injectIndex rule, what this should do is injecting index and not modifying anything on top of that.

ph
ph reviewed May 27, 2020
View reviewed changes
x-pack/elastic-agent/CHANGELOG.asciidoc
- Correctly report platform and family. {issue}18665[18665]
- Clean action store after enrolling to new configuration {pull}18656[18656]
[Ingest Manager] Avoid watching monitor logs {pull}18723[18723]
- Avoid watching monitor logs {pull}18723[18723]
Copy link
Copy Markdown
Contributor

@ph ph May 27, 2020

Choose a reason for hiding this comment

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

double entry?

Copy link
Copy Markdown
Contributor Author

@michalpristas michalpristas May 27, 2020

Choose a reason for hiding this comment

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

no i'm just fixing format of previous one

@ph ph mentioned this pull request May 27, 2020
Closed
@ph ph requested a review from ruflin May 27, 2020 15:58
ph
ph approved these changes May 27, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM, test look complete and easy to remove.

@michalpristas michalpristas merged commit d37b598 into elastic:master May 27, 2020
michalpristas added a commit to michalpristas/beats that referenced this pull request May 28, 2020
@michalpristas
[Ingest Manager] Guard against empty stream.datasource and namespace (e…
13b7ab8 
…lastic#18769)

[Ingest Manager] Guard against empty stream.datasource and namespace (elastic#18769)
michalpristas added a commit to michalpristas/beats that referenced this pull request May 28, 2020
@michalpristas
[Ingest Manager] Guard against empty stream.datasource and namespace (e…
ff5aded 
…lastic#18769)

[Ingest Manager] Guard against empty stream.datasource and namespace (elastic#18769)
This was referenced May 28, 2020
Merged
Merged
michalpristas added a commit that referenced this pull request May 28, 2020
@michalpristas
Cherry-pick #18769 to 7.8: Guard against empty stream.datasource and …
b17a3cf 
…namespace (#18806)

* [Ingest Manager] Guard against empty stream.datasource and namespace (#18769)
michalpristas added a commit that referenced this pull request May 28, 2020
@michalpristas
Cherry-pick #18769 to 7.x: Guard against empty stream.datasource and …
90e9630 
…namespace (#18805)

Cherry-pick #18769 to 7.x:  Guard against empty stream.datasource and namespace (#18805)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ruflin ruflin Awaiting requested review from ruflin

1 more reviewer

@ph ph ph approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@michalpristas michalpristas

Labels

bug Ingest Management:alpha1 Group issues for ingest management alpha1 needs_backport PR is waiting to be backported to other branches. review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@michalpristas @elasticmachine @ph @ruflin