-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Filebeat Azure - Add missing var definitions in the manifest files #16468
Conversation
- name: connection_string | ||
- name: storage_account | ||
- name: storage_account_key | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line here.
Thank you for fixing this so quickly! I think we should have a system test to make sure this doesn’t happen again. This would probably caught it, right? https://github.com/elastic/beats/blob/master/filebeat/tests/system/test_setup.py, probably we can load it also from x-pack Filebeat. Please update the description, even if the original issue has lots of details, it’s nice to have a good summary here explaining the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@narph Under configs in each azure fileset like https://github.com/elastic/beats/blob/master/x-pack/filebeat/module/azure/activitylogs/config/azure-eventhub.yml, the parameters that are not required (like connection_string, storage_account) needs to be checked to see if they are defined first. For example,
{{ if .connection_string }}
connection_string: {{ .connection_string }}
{{ end }}
Please see Andrew's comment in a similar PR: #15656 (comment)
@exekias I figured that the existing module tests would have caught this case. But what I realized is that because we only use the Or maybe the the |
SGTM, let's get this in for the next release and work on the testing part later |
@kaiyan-sheng , all the 3 params are required in the azure module for each fileset in this case. Two of these options might not be mandatory in the azure-eventhub input in the future so I would not add the |
jenkins test this |
jenkins test this |
…lastic#16468) * Add vars * changelog * temp * line * update * take test out (cherry picked from commit da3584d)
…lastic#16468) * Add vars * changelog * temp * line * update * take test out (cherry picked from commit da3584d)
Should fix #16270.
Due to new checks on the configuration file keys in v7.6.0, when running:
users get:
due to missing to define some of the vars in the manifest file.
This PR does this.