Skip to content
This repository has been archived by the owner on Feb 4, 2025. It is now read-only.

Add test rules and serve them with update_rules_content.sh --include-test-rules #329

Merged

Conversation

epapbak
Copy link
Collaborator

@epapbak epapbak commented Nov 23, 2021

Description

  • When implementing notification services BDD testcases, the need for specifying rules with a concrete category of impact appeared. These "test rules" serve that purpose, and allow use to not disclose private data in the BDD tests.

  • The new rules have specific impact and likelihood that ensure we have one rule of each total risk category.

  • In order for the content-service to serve these new rules, the update_rules_content.sh script now accepts the --include-test-rules parameter. This is intended for local use only.

Type of change

  • Other (no change in code)

Testing steps

Please describe how the change was tested locally. If, for some reason, the testing was not done or not done fully, please describe what are the testing steps.

Checklist

  • make before_commit passes
  • updated documentation wherever necessary
  • added or modified tests if necessary
  • updated schemas and validators in insights-data-schemas in case of input/output change

Copy link
Collaborator

@joselsegura joselsegura left a comment

Choose a reason for hiding this comment

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

LGTM, just a question:

why are the rules implementation needed for the purpose of this PR? I mean, content-service won't do anything with rules implementation, it only uses rules content, so probably these files won't be never used

@epapbak epapbak force-pushed the test_rules_with_different_risks branch from 0522750 to e074829 Compare November 23, 2021 10:28
@epapbak
Copy link
Collaborator Author

epapbak commented Nov 23, 2021

LGTM, just a question:

why are the rules implementation needed for the purpose of this PR? I mean, content-service won't do anything with rules implementation, it only uses rules content, so probably these files won't be never used

Thanks for the feedback! Truth is I copied the tutorial rule's directory structure. I didn't test without the rules implementation. I'll test it, and if it doesn't break, I'll remove them.

@epapbak epapbak force-pushed the test_rules_with_different_risks branch from fbfa98d to 1c52864 Compare November 23, 2021 10:44
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2021

Codecov Report

Merging #329 (b66bf1f) into master (3f90738) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #329   +/-   ##
=======================================
  Coverage   72.58%   72.58%           
=======================================
  Files          11       11           
  Lines         580      580           
=======================================
  Hits          421      421           
  Misses        134      134           
  Partials       25       25           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f90738...b66bf1f. Read the comment docs.

update_rules_content.sh Outdated Show resolved Hide resolved
@epapbak epapbak force-pushed the test_rules_with_different_risks branch from 1c52864 to fd6072d Compare November 23, 2021 10:53
  - Remove unnecessary config.yaml file
  - use `node_id: null` in plugin
  - remove rule implementation files (not needed in content service)
  - style fixes
@epapbak epapbak force-pushed the test_rules_with_different_risks branch from fd6072d to b66bf1f Compare November 23, 2021 10:53
Copy link
Collaborator

@Bee-lee Bee-lee left a comment

Choose a reason for hiding this comment

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

LGTM!

@epapbak epapbak merged commit f6da6c9 into RedHatInsights:master Dec 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants