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 #21088: Reporting issues when using multiple directives of the same technique on a node #1353

Merged
merged 1 commit into from May 25, 2022
Merged

Fixes #21088: Reporting issues when using multiple directives of the same technique on a node #1353

merged 1 commit into from May 25, 2022

Conversation

ncharles
Copy link
Member

@ncharles ncharles added Trigger test Trigger jenkins build WIP and removed Trigger test Trigger jenkins build labels May 12, 2022
@ncharles
Copy link
Member Author

to be merged with Normation/rudder#4269

Fdall
Fdall reviewed May 12, 2022
View reviewed changes
tree/30_generic_methods/service_stopped.cf
"${report_data.method_id}_${name}" usebundle => call_method_classes_caller;
"${report_data.method_id}_${name}" usebundle => call_method_end("ncf_services_stop");
pass3.!needs_stop::
# Define success classes is check_running was not kept
"${report_data.method_id}_${name}" usebundle => _classes_success("${class_prefix}");
"${report_data.method_id}_${name}" usebundle => _classes_success("${report_data.report_id}");
Copy link
Contributor

Choose a reason for hiding this comment

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

looks good, but I feel like we will have issues sooner or later with newer methods calling non-leaf methods that are defining classes on the report_id. If we handle correctly the push pop process, should'nt we be able to always define classes on the method_id and only use the report_id variable as a match for reporting?

@Fdall Fdall mentioned this pull request May 13, 2022
Closed
@ncharles ncharles added Trigger test Trigger jenkins build and removed WIP Trigger test Trigger jenkins build labels May 18, 2022
@ncharles ncharles added Trigger test Trigger jenkins build and removed Trigger test Trigger jenkins build labels May 24, 2022
@ncharles ncharles force-pushed the bug_21088/reporting_issues_when_using_multiple_directives_of_the_same_technique_on_a_node branch from 29a2f26 to e2b1761 Compare May 25, 2022 09:38
@ncharles ncharles added Trigger test Trigger jenkins build and removed Trigger test Trigger jenkins build Ready for merge labels May 25, 2022
@ncharles
Copy link
Member Author

PR updated with a new commit

@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/ncf/pull/1353
-- Your faithful QA
Kant merge: "Science is organized knowledge. Wisdom is organized life."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/54776/console)

@VinceMacBuche
Copy link
Member

OK, squash merging this PR

@VinceMacBuche
Copy link
Member

you need to reabse this pr in one commit @ncharles

@ncharles ncharles force-pushed the bug_21088/reporting_issues_when_using_multiple_directives_of_the_same_technique_on_a_node branch from 666ac63 to 5ecf847 Compare May 25, 2022 12:46
@ncharles
Copy link
Member Author

PR rebased

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit c2cd5c7 into Normation:branches/rudder/7.1 May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fdall Fdall Fdall left review comments

@VinceMacBuche VinceMacBuche VinceMacBuche approved these changes

Assignees
No one assigned
Labels
Ready for merge Trigger test Trigger jenkins build
Projects
None yet
Milestone
No milestone
4 participants
@ncharles @Normation-Quality-Assistant @VinceMacBuche @Fdall