-
Notifications
You must be signed in to change notification settings - Fork 3
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
Introduce the ConfigurationIssue system #307
Conversation
Add the IConfigurationIssueInterface, and the ConfigurationIssue class and the ConfigurationIssueLevel and ConfigurationIssueTemplate enumerations. - Configuration issues are stored for configuration objects and for configuration value objects. - They are either of type info, warning or error. - Issues in configuration values are passed to the configuration on request, they are not stored there! This has changed to previous versions of Roddy. Implement the IConfigurationIssueInterface interface in Configuration, ConfigurationValue. - The interface adds the methods: hasWarnings/Errors, addWarning/Error, getWarnings/Errors. - getWarning/Error returns a copy of the underlying collections in its current implementations. Move ConfigurationTest to ConfigurationSpec, add a test for error and warning passthrough to the configuration object. Configuration class: - Rename Configuration.hasErrors to hasLoadErrors. - Groovyfy some code in the Configuration class. Analysis class: - Rename Analysis.java to Analysis.groovy and Groovyfy it. - Rebuild and extend the print errors and warning methods in Analysis class to match the new possibilites and make the code more clear. Changes to ConfigurationValue - Add constructor methods for int, float, double, bool to ConfigurationValue. - Enable auto-validation of ConfigurationValue objects upon their construction. - Groovyfy some the class. The Enumeration class has the method getDefaultCValueTypeEnumeration() now. The method will return a default list of available configuration value types and their default validators. Also did a code cleanup to Groovyfy it. Move ConfigurationValueValidator.java to ConfigurationValueValidator.groovy and cleanup the class. Implement the IConfigurationIssueInterface and add proper fields and methods. Rename DefaultValidator.java to DefaultValidator.groovy and Groovyvy Extend the DefaultValidator class with checkDollarSignUsage(). The method now stores its errors and warnings in their fields in the parent class. Add test for proper dollar character usage to the ConfigurationFactory test. Add ConfigurationIssueSpec test class. Add the DefaultValidatorSpec test class. Remove the ConfigurationTest class.
Added a new constructor for elevated values to the ConfigurationValue class. This also speeds up the code, as the validation result will just be taken from the parent value. Otherwise, an unnecessary revalidation would occur and they are quite expensive. Changed the toString of ConfigurationIssue to print out the message instead of some made up text. Added equals and hashCode to EnumerationValue to make them comparable. Add printErrorsAndWarnings() to Analysis to allow a single call to print out warnings, info messages and errors. Add detailed warnings and error output for condensed messages to the detailed logfile. Added more tests.
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.
First, nice summary of the changes. Thanks!
Not fully done yet, but here are some comments.
value | type | expected | expectedWarnings | expectedErrors | ||
'abc${var}bcd${var}' | 'string' | true | [] | [] | ||
'abc${var}bcd${var}def' | 'string' | true | [] | [] | ||
'abc$' | 'string' | false | [unattachedDollarCharacter.expand('bla')] | [] |
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.
What about escaped dollar signs here?
'\\$' | 'string' | true | [] | []
'abc\\$' | 'string' | true | [] | []
'abc\\${' | 'string' | true | [] | []
'a\\${b}c' | 'string' | true | [] | []
What about "empty" dollar signs, i.e. dollars followed by non-variable characters?
'$' | 'string' | false | ["lonely variable initializer '$'"] | []
'abc$\\{' | 'string' | false | ["lonely variable initializer '$'"] | []
What about unmatched braces?
'ab${c' | 'string' | false | ["non-terminated variable expression"] | []
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.
Another -- aberrant -- input to check:
'${}' | 'string' | true | ["empty variable expression"] | []
This case is actually no so unlikely to happen.
a='{'
b='}'
c='\\$${a}${b}'
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.
I did a lot here. Check it out.
RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationFactoryTest.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationFactoryTest.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationFactoryTest.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/ConfigurationFactoryTest.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/src/de/dkfz/roddy/config/validation/DefaultValidator.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/src/de/dkfz/roddy/config/validation/DefaultValidator.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy
Outdated
Show resolved
Hide resolved
ConfigurationValue: - Fix the constructor for "double" values - Add the replaceObsoleteVariableIdentifiers and call it from the constructor. The method replaces several older malformed values ($VAL) to their proper form (${VAL}). - Remove the methods: * replaceString * toFile(), toFile(Map<String, String> replacements) * replaceConfigurationBasedValues * toString(List<String>) - Consolidate and cleanup the methods toFile(Analysis, DataSet) and toFile(Analysis). The methods now use the normal variable replacement feature of Roddy instead of the removed replace method - Make the field variableDetection static final - Make toEvaluatedValue final and extend its argument list Add a on-null-check to DefaultValidator.checkDollarSignUsage Analysis: - Clean up and groovyfy the class - Change the getConfiguration method so that it will return a configuration which knows values like USERNAME and USERGROUP DataSet now contains a getConfiguration method, which knows the different variants of DATASET. For internal use only! Add tests for variable replacement to ConfigurationValueSpec Add ExecutionService and FileSystemAccessProvider init code to some test classes
value | type | expected | expectedWarnings | expectedErrors | ||
'abc${var}bcd${var}' | 'string' | true | [] | [] | ||
'abc${var}bcd${var}def' | 'string' | true | [] | [] | ||
'abc$' | 'string' | false | [unattachedDollarCharacter.expand('bla')] | [] |
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.
Another -- aberrant -- input to check:
'${}' | 'string' | true | ["empty variable expression"] | []
This case is actually no so unlikely to happen.
a='{'
b='}'
c='\\$${a}${b}'
static enum ConfigurationIssueTemplate { | ||
unattachedDollarCharacter( | ||
ConfigurationIssueLevel.CVALUE_WARNING, | ||
"Several variables in your configuration contain one or more dollar signs. As this might impose problems in your cluster jobs, check the entries in your job configuration files. See the extended logs for more information.", |
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.
Yes, these values should not get warnings. The second option you propose I do not understand, but certainly users should not get worried unnecessarily.
unattachedDollarCharacter( | ||
ConfigurationIssueLevel.CVALUE_WARNING, | ||
"Several variables in your configuration contain one or more dollar signs. As this might impose problems in your cluster jobs, check the entries in your job configuration files. See the extended logs for more information.", | ||
"The variable named '#REPLACE_0#' contains one or more dollar signs, which do not belong to a Roddy variable definition (\${variable identifier}). This might impose problems, so make sure, that your results job configuration is created in the way you want." |
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.
Also: Too wordy message.
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.
New issue
/** | ||
* API Level 3.4+ | ||
*/ | ||
final boolean invalid |
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.
?
/** | ||
* API Level 3.4+ | ||
*/ | ||
ConfigurationValue(Configuration config = null, String id, boolean value) { |
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.
o.k. clarified -- Groovy idiom.
RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy
Outdated
Show resolved
Hide resolved
ConfigurationIssue: - Changed some messages in regards to the Pull Request - Added the inproperVariableExpression issue - Change name of unattachedDollarCharacter to detachedDollarCharacter ConfigurationValue: - Moved the getDefaultCValueTypeEnumeration from Enumeration to here - Make "invalid" field deprecated and remove it internally from isInvalid DefaultValidator: - Changed name of checkDataTypes to isConfigurationValueTypeCorrect - Add another check for variable imports - Change name of checkDollarSignUsage to areDollarCharactersProperlyUsed - Add tests for the new method
ConfigurationValue: - Fix the constructor for "double" values - Add the replaceObsoleteVariableIdentifiers and call it from the constructor. The method replaces several older malformed values ($VAL) to their proper form (${VAL}). - Remove the methods: * replaceString * toFile(), toFile(Map<String, String> replacements) * replaceConfigurationBasedValues * toString(List<String>) - Consolidate and cleanup the methods toFile(Analysis, DataSet) and toFile(Analysis). The methods now use the normal variable replacement feature of Roddy instead of the removed replace method - Make the field variableDetection static final - Make toEvaluatedValue final and extend its argument list Add a on-null-check to DefaultValidator.checkDollarSignUsage Analysis: - Clean up and groovyfy the class - Change the getConfiguration method so that it will return a configuration which knows values like USERNAME and USERGROUP DataSet now contains a getConfiguration method, which knows the different variants of DATASET. For internal use only! Add tests for variable replacement to ConfigurationValueSpec Add ExecutionService and FileSystemAccessProvider init code to some test classes
…com/theroddywms/Roddy into ChangeConfigurationValueReplacement
Change usage of strings like "string", "integer", etc. for configuration value types all over the code to use new constants in the ConfigurationConstants class. Changed minor things pointed at in the Pull request. ConfigurationValue: - Added a larger comment to toFile(). - Changed .equals() to ==. - Removed usage of checkAndCorrectPath and the method itself. - Moved getDefaultCValueTypeEnumeration from Enumeration to ConfigurationValue.
This class extends the Groovy Specification class and is used to do some basic initialization like the FileSystemProvier and the ExecutionService.
Updated the copyright notice in all files to our current version. Added the RoddyTestSpec class, which is meant to be a base class for Spock tests. The class provides some basic initialization settings.
The context object would have been available for every sub class. Removed it to avoid the possibility of paralellism issues. Just create your own context objects when you need them.
ConfigurationIssueLevel.CVALUE_WARNING, | ||
"Several variables in your configuration contain one or more dollar signs. As this might impose problems in your cluster jobs, check the entries in your job configuration files. See the extended logs for more information.", | ||
"The variable named '#REPLACE_0#' contains one or more dollar signs, which do not belong to a Roddy variable definition (\${variable identifier}). This might impose problems, so make sure, that your results job configuration is created in the way you want." | ||
"Variable '#REPLACE_0#' contains plain dollar sign(s) without braces. Roddy does not interpret them as variables and cannot guarantee correct ordering of assignments for such variables in the job parameter 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.
Maybe a hint how to avoid the warning? Maybe simply add "and unescaped" before dollar? Also "plain" in vague and does not add much meaning. Maybe better "unescaped dollar sign(s) without braces" would be sufficient.
"Variable '#REPLACE_0#' contains unescaped dollar sign(s) without braces. Roddy does not interpret them as variables and cannot guarantee correct ordering of assignments for such variables in the job parameter file."
/** | ||
* API Level 3.4+ | ||
*/ | ||
final boolean invalid |
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.
I don't understand. Why do you keep the "invalid" field anyway?
RoddyCore/src/de/dkfz/roddy/config/validation/DefaultValidator.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/src/de/dkfz/roddy/config/validation/DefaultValidator.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy
Outdated
Show resolved
Hide resolved
RoddyCore/test/src/de/dkfz/roddy/config/validation/DefaultValidatorSpec.groovy
Outdated
Show resolved
Hide resolved
Added a comment to Analysis.getConfiguration(). The method will fail for many tests, when the tests do not also initialize the ExecutionService and FileSystemAccessProvider. The comment describes the method which needs to be added to the test class. Added the aforementioned method to ContextResourceTest Changed ContextResource so, that the contextResource object is now @shared and a @ClassRule but NOT static! Also the static initializer in the anonymous creation is removed.
Add roddy test spec class and change all copyright notices
Add the SystemProperties class to add one place for accessing system properties and to perform code deduplication. Replace access code to System.getProperty() to use the new class. Update messages for two ConfigurationIssue templates. Extend the xml configuration part in the docs to describe the order of evaluation for ConfigurationValue.toFile(). Remove the invalid field and the setInvalid method from ConfigurationValue. Remove try/catch from getUsername() and getUsergroup() in the Analysis class. Both method returned "UNKNOWN". Change name of getOutputAnalysisBaseCV() to getOutputAnalysisBaseDirectoryCV() in RuntimeService class. Add the test case for $abc to 'test check proper dollar sign usage in variables' in DefaultValidatorSpec.
The DefaultValidator now recognizes starting '$' characters in strings. => '$abc' will be marked as invalid. Fix tests.
Please note, that I (unfortunately) did most of the fixes in #309. Please just check that and if you agree I will merge both. |
In DefaultValidator: - Rename areDollarCharactersProperlyUsed to areDollarCharactersAttached. - Typo fixes. Change messages in ConfigurationIssue class. Added a test for '${}' to "test check for proper variable usage definition"() in DefaultValidatorSpec test class. Fixed tests in ConfigurationIssueSpec test class.
…acement Fix for Configuration value replacement
Did all fixes in #309 and will merge now. |
Add the IConfigurationIssueInterface, and the ConfigurationIssue class
and the ConfigurationIssueLevel and ConfigurationIssueTemplate
enumerations.
configuration value objects.
request, they are not stored there! This has changed to previous
versions of Roddy.
Implement the IConfigurationIssueInterface interface in Configuration,
ConfigurationValue.
getWarnings/Errors.
current implementations.
Move ConfigurationTest to ConfigurationSpec, add a test for error and
warning passthrough to the configuration object.
Configuration class:
Analysis class:
class to match the new possibilites and make the code more clear.
Changes to ConfigurationValue
ConfigurationValue.
construction.
The Enumeration class has the method getDefaultCValueTypeEnumeration()
now. The method will return a default list of available configuration
value types and their default validators.
Also did a code cleanup to Groovyfy it.
Move ConfigurationValueValidator.java to
ConfigurationValueValidator.groovy and cleanup the class.
Implement the IConfigurationIssueInterface and add proper fields and
methods.
Rename DefaultValidator.java to DefaultValidator.groovy and Groovyvy
Extend the DefaultValidator class with checkDollarSignUsage(). The
method now stores its errors and warnings in their fields in the parent
class.
Add test for proper dollar character usage to the ConfigurationFactory
test.
Add ConfigurationIssueSpec test class.
Add the DefaultValidatorSpec test class.
Remove the ConfigurationTest class.