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

[Agent] Fix merge of config #17399

Merged
merged 3 commits into from
Apr 2, 2020

Conversation

michalpristas
Copy link
Contributor

@michalpristas michalpristas commented Apr 1, 2020

What does this PR do?

Copies every field from input to streams on input creation. Before this change only processors were copied.

Why is it important?

To allow specifying hosts or vars as a common configuration parts for input streams

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.

Fixes: #17377

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Project:fleet)

@ph
Copy link
Contributor

ph commented Apr 1, 2020

tested LGTM :)

@michalpristas michalpristas reopened this Apr 2, 2020
@michalpristas michalpristas merged commit 2654f72 into elastic:master Apr 2, 2020
- namespace: testing
use_output: default
inputs:
- type: apache/metrics
streams:
- enabled: true
metricset: info
hosts: ["http://apache.remote"]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is just a bad example? Because the same field should not exist in stream an input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes i just needed to test override functionality somehow

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't the conclusion we don't support override? @ph ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ruflin @michalpristas I think the conclusion was effectively we do not support overrides, I've looked at our email and this was the behavior he had in mind.

  • for keys defined in the input section we apply them in the streams.
  • for keys defined in the input section and present in the streams we fail hard.
  • If a key is not defined in the inputs but defined in the stream we take the value of the stream.

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 yes we don't this test is just to make sure nothing is replaced by anything defined at inputs level so we make sure it does not happen as we copy all the fields down the tree. where we need overrride are situation when we generate somethin in tree and we want to make sure it is not lost (e.g. processors) we generate on stream level (just like index) but then we merge it with top level processors (we cannot generate at top level as fields are stream specific)

Copy link
Member

Choose a reason for hiding this comment

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

Based on the comment from @ph , I would assume this test should fail hard? "for keys defined in the input section and present in the streams we fail hard."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think you're right, will make it fail entirely

@ph ph added the needs_backport PR is waiting to be backported to other branches. label Apr 7, 2020
@ph
Copy link
Contributor

ph commented Apr 8, 2020

backported in #17599

@ph ph removed the needs_backport PR is waiting to be backported to other branches. label Apr 8, 2020
ph pushed a commit to ph/beats that referenced this pull request Apr 10, 2020
ph added a commit that referenced this pull request Apr 14, 2020
)

* [Agent] Rename agent to elastic-agent (#17391)

[Agent] Rename agent to elastic-agent (#17391)

(cherry picked from commit 66609a3)

* Add Elastic Agent in the Jenkinsfile (#17445)

* Add Elastic Agent in the Jenkinsfile

With the recent moves to Jenkins pipeline the Elastic Agent project was
not currently tested.

* Add Elastic Agent in the Jenkinsfile (#17445)

* Add Elastic Agent in the Jenkinsfile

With the recent moves to Jenkins pipeline the Elastic Agent project was
not currently tested.

* [Agent] Fix merge of config (#17399)

[Agent] Fix merge of config #17399

* [Agent] Handle abs paths on windows correctly (#17461)

* handle windows correctly

* Missing commits from the initial merges of the agent -> master

Co-authored-by: Michal Pristas <michal.pristas@gmail.com>
Co-authored-by: Pier-Hugues Pellerin <phpellerin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Agent] The agent doesn't correctly merges items at the input level with and the stream level.
4 participants