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 #23652: iteration in technique editor with package method don't report correctly #1413

Draft
wants to merge 2 commits into
base: branches/rudder/7.3
Choose a base branch
from

Conversation

ncharles
Copy link
Member

@ncharles ncharles commented Oct 25, 2023

https://issues.rudder.io/issues/23652

This change adds a variability based on the key for generic methods.
To allow multiple execution of generic method based on service command and package, we introduced a stack for evaluation & reporting, that uses generic method id + directive + rule as an id
This solves the issue of multiple evaluations, yet it does not work with reporting: a list of package to install, with the first in repair and the other in success return repair for all the package, breaking the reporting

We have a facility to unwrap the list in the generated technique

27 bundle agent iteration_gm_1(c_name, c_key, report_id, name, version, architecture, provider) { 28 methods: 29 "c3974f2e-d9e9-4527-ab5f-e0add1c52318_${report_data.directive_id}" usebundle => _method_reporting_context_v4("${c_name}","${c_key}","${report_id}"); 30 "c3974f2e-d9e9-4527-ab5f-e0add1c52318_${report_data.directive_id}" usebundle => package_present("${name}","${version}","${architecture}","${provider}"); 31 }

which was introduced in https://issues.rudder.io/issues/20603

so, we could be insensitive to iteration if we add the key to the report
For packages, command and services, report_id and method_id are both used for defining classes & report condition (and I suspect that one in use in place of the other more than once); so adding in them the key allow unicity
This is was this change does and fixes the reporting issue

It needs to be more throughly tested, especially for the result condution of the GM, and for techniques, and services & command

@amousset
Copy link
Member

amousset commented Nov 2, 2023

Tests are failing with reporting-related errors.

@ncharles
Copy link
Member Author

ncharles commented Nov 2, 2023

ha, test framework probably needs to be updated as well, given the following error message
Missing method_id class 58abab1b_be8d_4809_b257_9113759ea5fd_d_58abab1b_be8d_4809_b257_9113759ea5fd_not_kept

@amousset
Copy link
Member

We will take this PR for 8.1, but replace the component arg with a hash of the parameters.

@fanf
Copy link
Member

fanf commented Jan 25, 2024

After more investigation, we need to do deeper change and create a logger_v5 to correctly handle the case. This is a too big change for 8.1, we are forced to switch it to 8.2.
Making it a draft.

@fanf fanf marked this pull request as draft January 25, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants