-
Notifications
You must be signed in to change notification settings - Fork 6
Support for YAML-based Files-To-Types mapping #70
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Mathieu Dalbin <[email protected]>
dennis-behm
left a comment
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.
@M-DLB please take a look to my comments. Let's take an extra iteration to clean up :-)
Also, please remove the type.txt from the sample directory
Setup.sh
Outdated
| REPOSITORY_PATH_MAPPING_FILE=$DBB_MODELER_WORK/config/repositoryPathsMapping.yaml | ||
| # Reference to the type mapping file | ||
| APPLICATION_MEMBER_TYPE_MAPPING=$DBB_MODELER_WORK/config/types/types.txt | ||
| APPLICATION_FILES_TYPES_MAPPING=$DBB_MODELER_WORK/config/types/filesToTypesMapping.yaml |
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.
We have the applicationsMapping.yaml, should the default file name be probably typesMapping.yaml. This would also fit with the typesConfiguration.yaml file.
samples/typesMapping.yaml
Outdated
| @@ -0,0 +1,19 @@ | |||
| files: | |||
| - file: "DBEHM.MIG.COBOL(LGACDB01)" | |||
| types: | |||
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.
Within the applicationDescriptor.yaml we end up with a single type field for the file. I am concerned about users to follow the train of thought, how things end up in a single variable then.
| } | ||
| def lastQualifier = getLastQualifier(dataset) | ||
| def memberType = fileUtils.getType(types, member.toUpperCase()) | ||
| def memberType = fileUtils.getType(filesToTypesMapping, datasetMember) |
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 see the need to externalise the getType() method. It is only invoked once. Can't we keep it inside of the extractApplications.groovy script?
src/groovy/utils/fileUtils.groovy
Outdated
| def type = types.get(member) | ||
| if (type) { | ||
| return type | ||
| def foundFile = filesToTypes.find { fileToTypes -> |
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 variable name "foundFile" is confusing. It's the list of types for the provided dataset(member)
src/groovy/utils/fileUtils.groovy
Outdated
| fileToTypes.file.equalsIgnoreCase(file) | ||
| } | ||
| if (foundFile) { | ||
| return foundFile.types.toString().replaceAll("\\[", "").replaceAll("\\]", "").replaceAll(" ", "") |
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 we keep the idea of allowing multiple types ... could we consider of sorting the list? And instead of replaceAll() a better way would be to join them
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.
Can you add a comment in this sample about the structure of it, and how it relates to the input dataset for the migration?
| types = fileUtils.loadTypes(props.APPLICATION_MEMBER_TYPE_MAPPING) | ||
| } | ||
| if (props.APPLICATION_FILES_TYPES_MAPPING) { | ||
| filesToTypesMapping = fileUtils.loadFilesToTypesMapping(props.APPLICATION_FILES_TYPES_MAPPING) |
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 see the need to externalise the loadFilesToTypesMapping() method into the utilities. It is only invoked once. Can't we keep it inside of the extractApplications.groovy script?
Signed-off-by: Mathieu Dalbin <[email protected]>
Signed-off-by: Mathieu Dalbin <[email protected]>
Signed-off-by: Mathieu Dalbin <[email protected]>
dennis-behm
left a comment
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.
@M-DLB Thank you for considering the comments and suggestion. This PR will give a more consistent experience about the MM configuration for the types!
This PR adds support for YAML-based mapping of files with their respective types. This enhancement helps to avoid collisions that could occur in the previous format, where programs or artifacts of different nature could have the same member name. Introducing the fully qualified member name with its owning dataset avoids this collision, and helps for an accurate classification of files to their defined types.
This PR doesn't break the concept of a file having multiples types assigned, combining them into one single type when creating build configuration. When a file has multiple types assigned, the types names are concatenated with dashes, and a specific type configuration is created by merging properties from each type used in the concatenation.