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 #20603: Reports on method using iterator are wrong in the cli output #4334

Conversation

ncharles
Copy link
Member

@ncharles ncharles commented Jun 17, 2022

https://issues.rudder.io/issues/20603

This PR changes the way ncf techniques are generated to support (at least partially) iterators.
The main idea is to create a bundle that will set the context and call the effective generic method. This is necessary so that any iterator within the arguments of the generic method are not iterated once for the reporting context, and then once for the generic method

One bundle is created by generic method, named by the promiser of the generic method plus an increment id.

The resulting code looks like

    "id1_${report_data.directive_id}" usebundle => id1_0("Customized component", "${node.properties[apache_package_name]}", "id1", "${node.properties[apache_package_name]}", "2.2.11"),
                                             if => concat("(debian)");`

and the bundle doing the action being

bundle agent id1_0(arg_0, arg_1, arg_2, arg_3, arg_4) {
  methods:
    "id1_${report_data.directive_id}" usebundle => _method_reporting_context_v4("${arg_0}","${arg_1}","${arg_2}");
    "id1_${report_data.directive_id}" usebundle => package_install_version("${arg_3}","${arg_4}");

}

@VinceMacBuche
Copy link
Member

I think the name of the parameter in the "id" method should me more explicit, ie the name of the method parameters for real parameter, and for the "reporting" value some more explicite name (component, value, id), it would be more explicit when debugging. (but here a problem on name colision, if there is a paramter that is name "id" or "value"

Also the name og the bundle could be different like "call__index" but it's ok.

I think i'll merge this like this and we may do another pr (or not)


val argsList = bundleArguments.zipWithIndex.map {case (_, id) => "arg_" + id}

val bundleName = (call.id + "_" + bundleIncrement).replaceAll("-", "_")
Copy link
Member Author

Choose a reason for hiding this comment

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

this may be not unique. We ought to prefix it with the technique name probably
Most likely technique_name_generic_method_name_incr would be more suited

Copy link
Member

Choose a reason for hiding this comment

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

what would make this not unique ? rudder_reporting ?

Copy link
Member Author

Choose a reason for hiding this comment

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

call.id is the same over different techniques
so you end up with bundles with the same name, and so it cannot be interpreted

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 have them the same, they should be generated at each method call ? which is not generating a new id ?

Copy link
Member

Choose a reason for hiding this comment

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

we were having problems of non execution due to call.id being reused when the technique was applied twice, and the problem was that the promiser was not unique

Copy link
Member

Choose a reason for hiding this comment

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

but the technique file will be created once

@ncharles
Copy link
Member Author

ncharles commented Jul 4, 2022

PR updated with a new commit

@VinceMacBuche
Copy link
Member

VinceMacBuche commented Jul 6, 2022

If found several issues, especially the use of the wrong bundle (effective bundle and not log_na_rudder in rudder_reporting.cf) so rudder_reporting was doing things and not just only make a na_report

I fixed some of them in
ncharles#12

@ncharles ncharles added the WIP Use that label for a Work In Progress PR that must not be merged yet label Jul 7, 2022
@ncharles
Copy link
Member Author

ncharles commented Jul 7, 2022

there is still an issue with log_na

@ncharles
Copy link
Member Author

ncharles commented Jul 7, 2022

PR updated with a new commit

@VinceMacBuche VinceMacBuche force-pushed the bug_20603/reports_on_method_using_iterator_are_wrong_in_the_cli_output branch from 27693ed to 89f5804 Compare July 7, 2022 09:34
@ncharles ncharles removed the WIP Use that label for a Work In Progress PR that must not be merged yet label Jul 7, 2022
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 6fb0cb1 into Normation:master Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants