Docker update extension - #117
Conversation
merging master
remove logging removed debug logging removed debug commits retrieve response as object and handle it
…cker-update-extension
rwm
left a comment
rwm
left a comment
There was a problem hiding this comment.
bitte auch nochmal Alexander drüber schauen lassen
| } | ||
|
|
||
| private boolean executeSocketOperation(int port) { | ||
| String host = getHost(); |
There was a problem hiding this comment.
warum ist der Host bekannt, aber nicht der Port?
There was a problem hiding this comment.
In der Realisierung eines Update Managers ist der Host konstant. Aber diese Methode wird dazu verwendet, verschiedene Ports anzusprechen. Darum kann der Host immer automatisch resolved werden, aber der Port muss zur jeweiligen Operation spezifiziert werden. Beispielsweise ruft der Debian Update Manager die Methode mit dem Port 1002 auf, wird das Apt-Update Socket angesprochen. Übergibt es nun den Port 1003 wird das DWH Update Socket angesprochen.
| * for the socket-based update agent communication as well as the environment | ||
| * detection used by {@link UpdateManagerFactory}.</p> | ||
| */ | ||
| public abstract class AbstractUpdateManager implements IUpdateManager { |
There was a problem hiding this comment.
Is there another implementation of IUpdateManager? If not, why the split then?
There was a problem hiding this comment.
The code can work without the interface Updater. With the interface in addition to the implementations, we get a logic layer that can be useful when it comes to creating other updaters not inheriting from AbstractUpdater. In this case it makes it easier to add other types of updaters to our code (open principle, or at least that was the idea).
There was a problem hiding this comment.
But does the abstraction right now reduce complexity or maintainability? Are other updaters already planned?---> YAGNI
| boolean supports = this.supportsCurrentSystem(); | ||
| LOGGER.log(Level.INFO, "Supports Current System: " + supports); | ||
| if (supports) { | ||
| reloadAptPackageLists(); |
There was a problem hiding this comment.
Why is this here? There should be either docker installation or deb installation
There was a problem hiding this comment.
This method still used the old naming for apt updates of debian dwhs. I changed it to refreshUpdateStatus() to make it generic. Also adapted the javadoc accordingly inside Updater.java
There was a problem hiding this comment.
Generic renaming would be a good idea. Is it already commited?
| @Override | ||
| public boolean supportsCurrentSystem() { | ||
| return Files.exists(Paths.get("/.dockerenv")); | ||
| } |
There was a problem hiding this comment.
Other way around. A parent class should do this check and then decide which updatemanager to use
There was a problem hiding this comment.
I can implement that, but this would go against dependency-inversion principle, by the parent requiring knowledge of lower classes. It would also mean, we would have to change the code inside the parent class, to implement new realisations of Updater, going against the open-close principle.
Currently to add a new realisation of Updater, we would only need to create a class implementing Updater or inheriting from AbstractUpdater and give it the annotation @EnvironmentSpecific. This automatically loads it as a singleton bean and integrates ito the logic seeminglessly.
There was a problem hiding this comment.
DIP isnt about inheritance relationships, but that abstractions (interfaces) should not depend on details (implementations). With the parent manager and the child agent we would effectively have the factory pattern. The current code is rigid, because it was only designed for apt environments without debian in mind. Thats why some rewrites are necessary
| @Override | ||
| public boolean supportsCurrentSystem() { | ||
| return !Files.exists(Paths.get("/.dockerenv")); |
There was a problem hiding this comment.
Why are two classes doing this check?
There was a problem hiding this comment.
The two classes are different implementations of Updater. Each one self-responsively checks their compatibility to the current system. Currently debian-updater checks, if dockerenv does not exist and docker-updater check if it exists. I need to overhaul the logic for debian-updater tough, do you have an idea how to check if the current environment is a debian installation?
| */ | ||
|
|
||
| @Singleton | ||
| public class UpdateManagerFactory { |
There was a problem hiding this comment.
A factory should be used for easy class initialization at runtime. In this case, the update manager need only to be initialized at startup once. A Service class could do that and the factory would not be necessary
There was a problem hiding this comment.
I changed the class name to UpdaterManager to clearify its function. The updater-beans are loaded automatically into the environment on application start up. This class finds them and returns the first updater-bean that supports the current environment.
3 participants
Backend now supports DWH updates for docker distributions, using the new version of AKTIN update agent aktin/debian-updateagent-pkg@main...docker-support.
Now different update handlers can be implemented in a modular and self-managed architecture, keeping changes to the actual DWH endpoints and function calls minimal.
The changes require a internal gateway passed inside compose.yml, which is also a new feature
aktin/docker-aktin-dwh@main...docker-update-extension