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

Check all options are valid and present in validate_cfg #373

Merged
merged 4 commits into from
Oct 13, 2020

Conversation

ajstewart
Copy link
Contributor

At the top of the config validation I've changed it such that the first thing that takes place is a check for:

  1. All config settings passed by the user are valid options.
  2. All valid options are present in the config file.

I think it's good to enforce this as then the user shouldn't have any surprise settings that they weren't aware of if they somehow created their own config file.

This also avoids the issue in #365 where any not defined options needed later in the process are caught at the beginning and hence doesn't waste any time.

As this full check is now in place I have removed a few checks that were there checking for individual settings.

Fixes #365.

@ajstewart ajstewart self-assigned this Oct 6, 2020
@github-actions github-actions bot added this to In progress in Pipeline Backlog Oct 6, 2020
Copy link
Contributor

@srggrs srggrs left a comment

Choose a reason for hiding this comment

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

I see that you want to enforce even options that would be optional if the method is not selected: for example if force extraction is turned off, we do not need the MONITOR_* options in the config.

I think this is fine, as long as we all on the same page @ajstewart @marxide

@ajstewart
Copy link
Contributor Author

I see that you want to enforce even options that would be optional if the method is not selected: for example if force extraction is turned off, we do not need the MONITOR_* options in the config.

I think this is fine, as long as we all on the same page @ajstewart @marxide

Yes that was on purpose, as the template should be used when creating jobs all options should be present in any case, and some parameters are still checked in the code when perhaps the user didn't think about the option. We can make this clear in documentation.

Pipeline Backlog automation moved this from In progress to Reviewer approved Oct 12, 2020
srggrs
srggrs previously approved these changes Oct 12, 2020
Pipeline Backlog automation moved this from Reviewer approved to Review in progress Oct 13, 2020
@ajstewart ajstewart requested a review from srggrs October 13, 2020 09:28
Pipeline Backlog automation moved this from Review in progress to Reviewer approved Oct 13, 2020
@ajstewart
Copy link
Contributor Author

I see that you want to enforce even options that would be optional if the method is not selected: for example if force extraction is turned off, we do not need the MONITOR_* options in the config.

I think this is fine, as long as we all on the same page @ajstewart @marxide

Are you happy with this validation method @marxide?

@ajstewart ajstewart merged commit 9a564cb into master Oct 13, 2020
Pipeline Backlog automation moved this from Reviewer approved to Done Oct 13, 2020
@ajstewart ajstewart deleted the issue-365-config-check branch October 13, 2020 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

CREATE_MEASUREMENTS_ARROW_FILE config not picked up if missing by config validation
3 participants