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 #24085: We need a consistant presence of policyMode in compliance API outputs #5357

Conversation

clarktsiory
Copy link
Contributor

@clarktsiory clarktsiory commented Jan 26, 2024

https://issues.rudder.io/issues/24085

Basically just adding the policyMode field at every level of the compliance tree.

For nodes and directives, policyMode is already an attribute, but for a rule we need to compute it, so this is a bit less performant as it requires the set of node ids by group to be fetched.

@clarktsiory clarktsiory requested a review from fanf January 26, 2024 16:02
Comment on lines +780 to 787
nodeTmpInfos <- onlyNode match {
case None => allNodeInfos.succeed
case Some(id) =>
allNodeInfos
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upmerge will be a little scary on this one because of the NodeInfoService to NodeFactsRepo migration, but the same implementation already exists in 8.1...

Apart from this block, there should be nothing that is non-complicated to change ^^

@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

1 similar comment
@clarktsiory
Copy link
Contributor Author

PR updated with a new commit

// A map to access the node fqdn and settings
val nodeInfos = nodeTmpInfos.view.mapValues(n => (n.name, n)).toMap

val nodeAndPolicyModeByRules = rules
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 we shouldn't have a cache for that.
The "node/rule policy mode" is something that is costly to compute, and vary only at specific point (new node, change on rule/directive, change on groups - that's all?). So it looks like a good candidate to keep these data. I think actually the changes are the same than when a generation is needed, and the same than what we would like to have in the expected reports.

So, perhaps not something for that PR, but we need to keep in mind that when perf will need to get better

Copy link
Member

@fanf fanf left a comment

Choose a reason for hiding this comment

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

LGTM, tests are consistant and datastructures are homogeneous between rules and nodes prism.
The performance impact is complicated to assess, it will need a specific task if needed.

@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/5357
-- Your faithful QA
Kant merge: "Two things awe me most, the starry sky above me and the moral law within me."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/79409/console)

@fanf
Copy link
Member

fanf commented Jan 29, 2024

OK, squash merging this PR

@fanf fanf force-pushed the bug_24085/we_need_a_consistant_presence_of_policymode_in_compliance_api_outputs branch from 6e1db8b to 462ce9b Compare January 29, 2024 11:37
@fanf fanf merged commit 462ce9b into Normation:branches/rudder/8.0 Jan 29, 2024
6 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants