-
Notifications
You must be signed in to change notification settings - Fork 215
Multi-Asssignments feature removal #2893
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
Multi-Asssignments feature removal #2893
Conversation
Signed-off-by: strailov <[email protected]>
Signed-off-by: strailov <[email protected]>
avgustinmm
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.
LGTM, but since bigger commit - better have at least one more review
| "hawkbit.server.error.repo.tenantConfigurationValueChangeNotAllowed", | ||
| "The requested tenant configuration value modification is not allowed."), | ||
| SP_MULTIASSIGNMENT_NOT_ENABLED( | ||
| SP_MULTIASSIGNMENT( |
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.
isn't this now removed ?
"The requested operation requires multi assignment to be enabled" is not valid error anymore ?
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 but on one place - on deployment requests we still throw such exception (assignDistributionSets) - which was modified to multiAssignmentException. The idea behind is that we still may allow more than 1 assignment on a device - so I guess we should throw exception.
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.
RepositoryProperties not used anymore - was used for getActionWeight that is now removed
private int getWeightConsideringDefault(final Action action) { return action.getWeight().orElse(repositoryProperties.getActionWeightIfAbsent()); }
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.
Will check that.
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.
many unused imports after removals
| */ | ||
| @Test | ||
| void getRollout() throws Exception { | ||
| enableMultiAssignments(); |
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.
strange why it was needed
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.
Strange indeed - it was used on couple of places without need.
| final boolean bypassWeightEnforcement = true; | ||
| final boolean multiAssignmentsEnabled = TenantConfigHelper.isMultiAssignmentsEnabled(); | ||
| if (!bypassWeightEnforcement && multiAssignmentsEnabled && hasNoWeight) { | ||
| if (!bypassWeightEnforcement && hasNoWeight) { |
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.
isn't this always false ?
bypassWeightEnforcment = true
if(!bypassWeightEnforcment && ..
It seems this validateWeight was used only when multiAssignemntEnabled= true ?
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.
and hasWeight is actually not used ?
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.
need to revise but it seems this was only needed when multiassignment was enabled, otherwise not
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.
Indeed - left it because there was a comment above - so I guess it was for a revision and decided not to change non relevant logic for the PR.
Signed-off-by: strailov <[email protected]>
|



Multi-Assignments feature removal
Removed all DMF related events - MultiActionAssignEven, MultiActionEvent, MultiActionCancelEvent (also related service events).
Renaming MultiAssignmentIsNotEnabledException to MultiAssignmentException (Thrown when multi-assignment is attampted in a single DeploymentRequest) .
Adapting tests accordingly.