-
Notifications
You must be signed in to change notification settings - Fork 74
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 #3967 : Improve change request merge check: #326
Fixes #3967 : Improve change request merge check: #326
Conversation
I started by refactoring all the logic in one method, so the logic is no more repeated, but I don't really like the result ... |
François, if it's ok tell me so i can rebase it! |
) extends Changes[Directive, ChangeRequestDirectiveDiff, DirectiveChangeItem] { | ||
val changes = OnlyDirectiveChange(directivesChanges.changes) | ||
val changeHistory = directivesChanges.changeHistory.map(OnlyDirectiveChange) | ||
} | ||
//////////////////// Node Part ////////////////////////////////////////////// |
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'm not sur I can agree with that. If the change request needs Technique, Directive and SectionSpec, I don't wan't to limitate the comparison check to Directive. What happens if the sectionSpec changed ?
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 section spec has changed, it means that parameters of the Directive have changed and/or the Technique version has changed, so the unmergability will be found)
The Technique can't changed, (or only if LDAP/archive data has been checked so they don't need to be checked)
We only keep the root section and the technique to be able to display the value in the webapp, and are used to verify that parameters are well formed
for the merge, we were onoly compaiting directive, i wanted to restrict to the same level.
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.
OK, so with some more explanation, I would like to have these two classes as internal, hidden tools of the CommitAndDeployChangeRequestServiceImpl class (or even of the methods actually using 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.
I can't move them... Traits Change and Changes are sealed so we can't create them outside this 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.
We decided to use an Option instead of a Change, much easier too read and to use!
Ldap datas are transformed in XML, then checked, before being unserialised to their types
…lize_ldap_into_xmll Fixes #3967 : Improve change request merge check:
Ldap datas are transformed in XML, then checked, before being
unserialised to their types
http://www.rudder-project.org/redmine/issues/3967