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.

Sign up for GitHub

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

Jump to bottom

Fixes #22552: General improvements on Directive compliance API #4720

Merged
merged 1 commit into from
Apr 4, 2023
Merged

Fixes #22552: General improvements on Directive compliance API #4720

merged 1 commit into from
Apr 4, 2023

Conversation

VinceMacBuche
Copy link
Member

@VinceMacBuche VinceMacBuche requested a review from fanf March 28, 2023 18:25
@VinceMacBuche VinceMacBuche added the WIP Use that label for a Work In Progress PR that must not be merged yet label Mar 28, 2023
ncharles
ncharles reviewed Mar 28, 2023
View reviewed changes
.../rudder-core/src/main/scala/com/normation/rudder/services/reports/ReportingServiceImpl.scala Outdated
for {
time_0 <- currentTimeMillis
nodeIds <- nodeConfigService.findNodesApplyingDirective(directiveId)
rules <- rulesRepo.getAll()
Copy link
Member

Choose a reason for hiding this comment

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

you don't use the rules

@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from b7e6717 to 0445d2f Compare March 28, 2023 21:32
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch 2 times, most recently from f00c9bd to d3f10bf Compare March 28, 2023 21:44
@VinceMacBuche
Copy link
Member Author

Commit modified

1 similar comment
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from d3f10bf to 90a109e Compare March 29, 2023 10:51
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from 90a109e to 1cd1f45 Compare March 29, 2023 11:37
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from 1cd1f45 to f54d14b Compare March 29, 2023 11:45
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from f54d14b to fbd54ab Compare March 29, 2023 14:36
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from fbd54ab to 2030977 Compare March 29, 2023 15:13
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from 2030977 to 2efc650 Compare March 29, 2023 15:15
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from 2efc650 to e1773a8 Compare March 29, 2023 15:28
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from e1773a8 to efc8397 Compare March 29, 2023 21:04
@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from efc8397 to 3706d43 Compare March 30, 2023 15:41
fanf
fanf reviewed Mar 31, 2023
View reviewed changes
...r/rudder-core/src/main/scala/com/normation/rudder/repository/ExpectedReportsRepository.scala
* only limited on the nodeIds in parameter (used when cache is incomplete)
*/
def findCurrentNodeIdsForDirective(ruleId: DirectiveId, nodeIds: Set[NodeId]): IOResult[Set[NodeId]]
def findCurrentNodeIdsForDirective(ruleId: DirectiveId): IOResult[Set[NodeId]]
Copy link
Member

Choose a reason for hiding this comment

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

you should not use the same function name for different signatures, it creates problems latter on (inference, refactoring, search not working as expected, etc).
At least, each and every time I thought it was a good idea, I got bitten down the path latter on

Copy link
Member

Choose a reason for hiding this comment

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

Actually, given the code it could be that nodeIds is an option (or have the semantic "if set is empty, ignore id") and just add the and fragment when it's not empty

Copy link
Member Author

Choose a reason for hiding this comment

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

What is complex is that to keep the current behaviour:
If some nodes ids => Make the request with a where condition on nodeIds
If none => Make the request without where on node ids
If Some empty set => Don't make request

I'm ok that None and some(Empty) makes sense to not have the and So i need to look the semantic of the empty Set here

And yes i would totally love rewriting this query using more doobie fragments and tools

So i may do another PR to improve this

fanf
fanf reviewed Mar 31, 2023
View reviewed changes
...core/src/main/scala/com/normation/rudder/repository/jdbc/ExpectedReportsJdbcRepository.scala
select distinct nodeid from nodeconfigurations
where enddate is null and configuration like ${"%" + directiveId.serialize + "%"}
""".query[NodeId].to[Set].transact(xa))
}
Copy link
Member

Choose a reason for hiding this comment

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

can you factor out the common bit? I think I saw you use something for and part somewhere

fanf
fanf reviewed Mar 31, 2023
View reviewed changes
...sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/ComplianceApi.scala
(if (groupsComponents.isEmpty) {
Nil
} else {
val bidule = groupsComponents.flatMap { case (nodeId, c) => c.subComponents.map(sub => (nodeId, sub)) }
Copy link
Member

Choose a reason for hiding this comment

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

:)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the bidule was already there .... (but it was already me)

fanf
fanf reviewed Mar 31, 2023
View reviewed changes
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.

I would love some unit tests for findDirectiveNodeStatusReports and there is a bit of code to factor out, but it's small compared to the PR. My tests on dev data seems ok.
So you can merge it like that so that we have a shippable rudder next week, and add unit test along the way.

@VinceMacBuche
Copy link
Member Author

Commit modified

@VinceMacBuche VinceMacBuche force-pushed the bug_22552/general_improvements_on_directive_compliance_api branch from 3706d43 to 493dc54 Compare April 3, 2023 21:58
@VinceMacBuche VinceMacBuche added Ready for merge and removed WIP Use that label for a Work In Progress PR that must not be merged yet labels Apr 3, 2023
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit fc4ddba into Normation:branches/rudder/7.3 Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanf fanf fanf left review comments

@ncharles ncharles ncharles left review comments

Assignees
No one assigned
Labels
Ready for merge
Projects
None yet
Milestone
No milestone
4 participants
@VinceMacBuche @Normation-Quality-Assistant @fanf @ncharles