-
Notifications
You must be signed in to change notification settings - Fork 186
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
Feature offline deployments #563
Feature offline deployments #563
Conversation
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
* Method registers an "offline" assignment, i.e. adds a completed action | ||
* for the given {@link DistributionSet} to the given {@link Target}s. | ||
* | ||
* The handling differs to hawkBit manged updates my means that:<br/> |
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.
Typo: manged => managed
* | ||
* @throws IncompleteDistributionSetException | ||
* if mandatory {@link SoftwareModuleType} are not assigned as | ||
* define by the {@link DistributionSetType}. |
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.
Typo: define => defined
* for the given {@link DistributionSet} to the given {@link Target}s. | ||
* | ||
* The handling differs to hawkBit manged updates my means that:<br/> | ||
* A. it ignores targets completely that are in |
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 you use a ordered list, you do not have to format it manually:
<ol type="A">
<li>it ignores targets completely that are in {@link TargetUpdateStatus#PENDING}</li>
<li>it creates completed actions</li>
....
</ol>
@@ -59,6 +59,29 @@ void setAssignedDistributionSetAndUpdateStatus(@Param("status") TargetUpdateStat | |||
@Param("lastModifiedBy") String modifiedBy, @Param("targets") Collection<Long> targets); | |||
|
|||
/** | |||
* Sets {@link JpaTarget#getAssignedDistributionSet()} and |
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.
Better:
Sets {@link JpaTarget#getAssignedDistributionSet()}, {@link JpaTarget#getInstalledDistributionSet()} and
{@link JpaTarget#getInstallationDate()}.
@@ -428,6 +428,38 @@ private JpaAction assignSet(final Target target, final DistributionSet ds) { | |||
return action; | |||
} | |||
|
|||
@Test | |||
@Description("Simple offline deployment of a distribution set to a list of targets. Verifies that offline assigment " | |||
+ "is correctly executed for targets that do not have a runnign update already. Those are ignored.") |
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.
Typo: runnign => running
.findAll(TargetSpecifications.hasControllerIdAndAssignedDistributionSetIdNot(ids, set.getId()))) | ||
.flatMap(List::stream).collect(Collectors.toList()); | ||
final List<JpaTarget> targets; | ||
if (offline) { |
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.
Due to the offline capability there are a lot of branches within the code now. This makes it hard to maintain and you'll get at lot of duplications. Have a look only at the public methods: All of them pass through a boolean to state whether it is an offline or online update (indicator for violating SRP). I suggest to either split this class into two internal classes (e.g. JpaOnlineDeploymentManagement
and JpaOfflineDeploymentManagement
) or make use of the strategy-pattern for online/offline case e.g.:
interface UpdateStrategy { // maybe there is a better name for it
List<JpaTarget> findTarget(final List<String> controllerIDs, final long setId);
void updateTargetStatus(final JpaDistributionSet set, final List<List<Long>> targetIds, final String currentUser);
Set<Long> findTargetIdToCancel(List<List<Long>> targetIds);
Status getActionStatus();
}
class OnlineUpdate implements UpdateStrategy{}
class OfflineUpdate implements UpdateStrategy{}
Later on in the implementation you won't have an if/else statement and only call:
strategy.findTarget(ids, set.getId());
Instead of:
final List<JpaTarget> targets;
if (offline) {
targets = Lists.partition(controllerIDs, Constants.MAX_ENTRIES_IN_STATEMENT).stream()
.map(ids -> targetRepository.findAll(SpecificationsBuilder.combineWithAnd(Arrays.asList(
TargetSpecifications.hasControllerIdAndAssignedDistributionSetIdNot(ids, set.getId()),
TargetSpecifications.notEqualToTargetUpdateStatus(TargetUpdateStatus.PENDING)))))
.flatMap(List::stream).collect(Collectors.toList());
} else {
targets = Lists.partition(controllerIDs, Constants.MAX_ENTRIES_IN_STATEMENT).stream()
.map(ids -> targetRepository.findAll(
TargetSpecifications.hasControllerIdAndAssignedDistributionSetIdNot(ids, set.getId())))
.flatMap(List::stream).collect(Collectors.toList());
}
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
return ResponseEntity | ||
.ok(MgmtDistributionSetMapper.toResponse(this.deployManagament.assignDistributionSet(distributionSetId, | ||
targetIds.stream() | ||
.map(t -> new TargetWithActionType(t.getId(), |
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.
It's better to use a descriptive value for the the parameter t (e.g. targetId)
import org.springframework.context.ApplicationEventPublisher; | ||
|
||
/** | ||
* @author kaizimmerm |
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.
Remove author and add javadoc to class
* | ||
* @param controllerIDs | ||
* as provided by repository caller | ||
* @param setId |
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.
Better call it distributionSetId
/** | ||
* Update status and DS fields of given target. | ||
* | ||
* @param set |
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.
Better call it distributionSetId here and in the method signature
/** | ||
* Cancels actions that can be canceled (i.e. | ||
* {@link DistributionSet#isRequiredMigrationStep() is <code>false</code>}) | ||
* as a result of the new assignment and returns all {@link Target}s whers |
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.
typo: where
return activeActions.stream().map(action -> { | ||
action.setStatus(Status.CANCELING); | ||
// document that the status has been retrieved | ||
|
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.
Remove line or move comment down
public DistributionSetAssignmentResult offlineAssignedDistributionSet(final Long dsID, | ||
final Collection<String> controllerIDs) { | ||
return assignDistributionSetToTargets(dsID, controllerIDs.stream() | ||
.map(t -> new TargetWithActionType(t, ActionType.FORCED, -1)).collect(Collectors.toList()), null, |
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.
Rename t to targetId or controllerId
return assignDistributionSetToTargets(dsID, targetIDs.stream() | ||
.map(t -> new TargetWithActionType(t, actionType, forcedTimestamp)).collect(Collectors.toList()), null); | ||
return assignDistributionSetToTargets(dsID, controllerIDs.stream() | ||
.map(t -> new TargetWithActionType(t, actionType, forcedTimestamp)).collect(Collectors.toList()), null, |
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.
Rename t to targetId or controllerId
Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
* Repository support offline deployments. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Add offline assignment to Management API. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * DsAssignmentStrategy introduced. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Fixed JavaDoc. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com> * Readibility improved. Signed-off-by: kaizimmerm <kai.zimmermann@bosch-si.com>
Support for offline deployment in repository and Management API.
That comes in handy for several scenarios, e.g.
Feature overview:
While at it I fixed a bug as well where assign DS response worked different on target and DS resources.
Reviewers: @schabdo @Jonkno