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 #7157: Use the new class_prefix in reporting #229

Conversation

peckpeck
Copy link
Member

@peckpeck peckpeck commented Sep 9, 2015

@peckpeck
Copy link
Member Author

peckpeck commented Sep 9, 2015

PR updated

@peckpeck peckpeck force-pushed the arch_7157/use_the_new_class_prefix_in_reporting branch from 185fcc5 to ea9d111 Compare September 9, 2015 15:49
@peckpeck
Copy link
Member Author

PR updated

@peckpeck peckpeck force-pushed the arch_7157/use_the_new_class_prefix_in_reporting branch from ea9d111 to 7898754 Compare September 10, 2015 07:56
"logging" usebundle => _bundle_caller_two("${configuration.enabled_loggers}", "${message}" , "${class_prefix}");

"${method}"
usebundle => ${method}("${message}", "${old_class_prefix}", "${args}"),
Copy link
Member

Choose a reason for hiding this comment

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

can't you directly ${configuration.enabled_loggers}("${message}", "${old_class_prefix}", "${args}") ?

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 can :)

@ncharles
Copy link
Member

This is really neat !
The logger_rudder would definitively need more comments for readability

@peckpeck peckpeck force-pushed the arch_7157/use_the_new_class_prefix_in_reporting branch from 7898754 to e68a0b9 Compare September 10, 2015 15:48
@peckpeck
Copy link
Member Author

PR updated


# 4/ Array is ready, reporting time !!!
"any" usebundle => _rudder_common_reports_generic("${report_data[1]}", "${c_class_prefix}", "${report_data[3]}", "${report_data[4]}", "${report_data[5]}", "${message}"),
classes => classes_generic("logger_rudder_${c_class_prefix}");

# We HAVE the data and we CAN dot it -> New reporting method
report_v2::
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be:
report_v2.report_data_read.!no_old_class_prefix:: ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No:

  • report_data_read is specific to report v1 so it is not needed here
  • no_old_class_prefix is used to remove v1 style reporting, but not v2 style

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 at least have logger_rudder_final_resfile no ?
Otherwise the file will not have been finished; or is it is not necessary, then in case of v2 report, the file .res should not be created

Copy link
Member

Choose a reason for hiding this comment

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

Or, can you have both v1 and v2 ?
What happen if the first report line is a v2 ?

Copy link
Member

Choose a reason for hiding this comment

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

ha, I understood, ok
report_v2 can only be put if expected_report_v2_res

@peckpeck
Copy link
Member Author

PR updated

@peckpeck peckpeck force-pushed the arch_7157/use_the_new_class_prefix_in_reporting branch from e68a0b9 to a31c7c5 Compare September 15, 2015 15:22
@ncharles
Copy link
Member

Yay !

ncharles added a commit that referenced this pull request Sep 18, 2015
…fix_in_reporting

Fixes #7157: Use the new class_prefix in reporting
@ncharles ncharles merged commit 69dec1f into Normation:master Sep 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants