Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

FIM System tests: Scenario 0207 report changes #546

Merged

Conversation

rshad
Copy link
Contributor

@rshad rshad commented Feb 26, 2020

Hi all!

This PR resolves #523 .

Kr,

Rshad

Manuel Gutierrez and others added 27 commits February 24, 2020 13:03
@rshad rshad added this to the Sprint 107 - DevOps milestone Feb 26, 2020
@rshad rshad requested a review from xr09 February 26, 2020 15:34
@rshad rshad requested review from Zenidd, DFolchA and jm404 February 26, 2020 15:34
@rshad rshad self-assigned this Feb 26, 2020
@rshad rshad changed the title Feature 523 scenario report changes FIM System tests: Scenario 0207 report changes Feb 26, 2020
Copy link
Contributor

@jm404 jm404 left a comment

Choose a reason for hiding this comment

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

Hi @rshad,

Please have a look at requested changes

@@ -9,7 +9,7 @@
- name: "Verify alert json log | {{ event }}"
script: >
verify_alerts_json.py -i {{ item.path }} -e {{ event }}
-o {{ missing_alerts_json_path }}
-o {{ missing_alerts_json_path }} {{ verify_json_script_extra_params }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please let's standardize this as json_verification_extra_arguments, as commented in #549 (comment)

@@ -21,7 +21,7 @@
script: >
verify_alerts_elasticsearch.py -i {{ item.path }} -e {{ event }}
-a {{ hostvars[groups["elasticsearch"][0]]["private_ip"] }}
-o {{ missing_alerts_elasticsearch_path }}
-o {{ missing_alerts_elasticsearch_path }} {{ verify_ES_script_extra_params }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's standardize this as elastic_verification_extra_arguments

@@ -0,0 +1 @@
agents_outputs
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed the scenario folder, is too long and doesn't describe the trigger, the syntax na me is id_description_trigger, please consider changing it to 207_report_changes_frequency

Copy link
Contributor

@jm404 jm404 left a comment

Choose a reason for hiding this comment

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

LGTM

@jm404 jm404 linked an issue Mar 2, 2020 that may be closed by this pull request
15 tasks
Copy link
Contributor

@jm404 jm404 left a comment

Choose a reason for hiding this comment

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

LGTM, pendant to folder rename and we can merge

@rshad rshad requested review from xr09 and jm404 March 2, 2020 21:24
@jm404 jm404 merged commit 38353ba into feature-493-create-test-scenarios Mar 2, 2020
@jm404 jm404 deleted the feature-523-scenario-report-changes branch March 2, 2020 21:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FIM System tests: 0207 Use of report changes
3 participants