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
Validate structure #753
Validate structure #753
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me
@pnax Thanks for reviewing. @matsduf Could you review this too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, but one comment.
$obj->{_ZONEMASTER_number_of_processes_for_frontend_testing} = $get_and_clear->( 'ZONEMASTER', 'number_of_processes_for_frontend_testing' ) // '20'; | ||
$obj->{_ZONEMASTER_number_of_processes_for_batch_testing} = $get_and_clear->( 'ZONEMASTER', 'number_of_processes_for_batch_testing' ) // '20'; | ||
$obj->{_ZONEMASTER_lock_on_queue} = $get_and_clear->( 'ZONEMASTER', 'lock_on_queue' ) // '0'; | ||
$obj->{_ZONEMASTER_age_reuse_previous_test} = $get_and_clear->( 'ZONEMASTER', 'age_reuse_previous_test' ) // '600'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there are default values in Config.pm the default configuration file should not have the same value.
[ZONEMASTER]
max_zonemaster_execution_time = 300
number_of_processes_for_frontend_testing = 20
number_of_processes_for_batch_testing = 20
#age_reuse_previous_test=600
The max_zonemaster_execution_time is different from the Config.pm. I suggest that the configuration file has the same values, but the key is commented, or that they are just removed from the configuration file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place I can find the value 300 is in the Travis backend config files. Do you think those must use the default value too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If the default values are set in Config.pm, the keys in the default configuration file should be commented as
age_reuse_previous_test
is. - In https://github.com/zonemaster/zonemaster-backend/blob/develop/share/backend_config.ini I find the the following:
[ZONEMASTER]
max_zonemaster_execution_time = 300
number_of_processes_for_frontend_testing = 20
number_of_processes_for_batch_testing = 20
#age_reuse_previous_test=600
I suggest that default backend_config.ini
is updated to:
[ZONEMASTER]
# max_zonemaster_execution_time = 600
# number_of_processes_for_frontend_testing = 20
# number_of_processes_for_batch_testing = 20
# age_reuse_previous_test=600
share/backend_config.ini
Outdated
number_of_processes_for_frontend_testing = 20 | ||
number_of_processes_for_batch_testing = 20 | ||
#age_reuse_previous_test=600 | ||
#age_reuse_previous_test = 600 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number_of_processes_*
keys have also default values in config.pm. I suggest them to be commented too:
# number_of_processes_for_frontend_testing = 20
# number_of_processes_for_batch_testing = 20
Context
This PR is a concrete step towards #685.
Scope
This PR concerns itself with what config properties are present and absent. It does not concern itself with the values of properties. Neither does it concern itself with the properties profile sections since those are more dynamic in nature.
Changes
This PR updates Zonemaster::Backend::Config->parse() to validate that all present properties are recognized and that all required properties are present.
This PR also updates Zonemaster::Backend::Config->parse() to defer all deprecation warnings so that they are only emitted if all strict validations pass. This should make future maintenance easier since it gives us more room to rearrange the validation code without affecting the behavior with regard to deprecation warnings.
How to test this PR
New unit tests are added for the relevant cases, so additional manual testing should not be necessary.