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

Fixes #24097: Skipped directive does not show in node compliance tree #5630

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Apr 25, 2024

https://issues.rudder.io/issues/24097

Essentially the same as #5600, and with some refactoring :

  • move the Elm model for SkippedDirective to the Elm Compliance.Datatypes module
  • compute the skipped directives in a method which takes the conversion DirectiveComplianceOverride => T as a param, either ByRuleDirectiveCompliance or ByRuleDirectiveCompliance (this PR)

Screenshot from 2024-04-25 17-48-22

@clarktsiory clarktsiory requested a review from fanf April 25, 2024 15:48
@fanf
Copy link
Member

fanf commented Apr 30, 2024

So, the code looks good and we now have skipped in node, but I still think there is something broken: in the case where the rule has only one directive and that directive is skipped, then the rule doesn't appear at all in the node.

Step to reproduce: in your screenshot example, remove the directive "File download (HTTP)" from rule "Install package skip"

@fanf
Copy link
Member

fanf commented May 6, 2024

I'm going to merge that one as it is, it's already a nice enhancement, and then I'm opening a new issue for the remaining problem for rule with all directive skipped.

@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/5630
-- Your faithful QA
Kant merge: "Science is organized knowledge. Wisdom is organized life."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/84211/console)

@clarktsiory
Copy link
Contributor Author

OK, merging this PR

@clarktsiory clarktsiory merged commit fbdc779 into Normation:branches/rudder/8.0 May 10, 2024
15 checks passed
@clarktsiory
Copy link
Contributor Author

OK, merging this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants