-
Notifications
You must be signed in to change notification settings - Fork 191
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
Split repository API for module and DS management. Refactor utility usage #524
Split repository API for module and DS management. Refactor utility usage #524
Conversation
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Conflicts: hawkbit-dmf/hawkbit-dmf-amqp/src/test/java/org/eclipse/hawkbit/integration/AmqpMessageHandlerServiceIntegrationTest.java hawkbit-repository/hawkbit-repository-jpa/src/test/java/org/eclipse/hawkbit/repository/jpa/ReportManagementTest.java
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Conflicts: hawkbit-repository/hawkbit-repository-core/pom.xml hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/RepositoryApplicationConfiguration.java
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
CONTRIBUTING.md
Outdated
@@ -21,6 +21,27 @@ Please read this if you intend to contribute to the project. | |||
* Sonarqube: | |||
* Our rule set can be found [here](https://sonar.ops.bosch-iot-rollouts.com/projects) with navigating to the tab "Quality Profiles", selecting "hawkBit", and then selecting "Actions" - "Back up" | |||
|
|||
### Utility library usage | |||
|
|||
hawkBit has currently both [guava](https://github.com/google/guava) and [Apache commons lang](https://commons.apache.org/proper/commons-lang/) on the classpath in several of its modules. However, we see introducing to many utility libraries problematic as we force these as transitive dependencies on hawkBit users. We in fact are looking into reducing them in future not adding new ones. |
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.
... introducing too many...
CQ13563 for using caffeine cache instead of guava. Allows to keep guava out of core. Besides Boot will switch to Caffeine as default anyhow. |
@@ -114,7 +113,7 @@ public DistributionSetBuilder description(final String description) { | |||
* @return a list of {@link MgmtDistributionSetRequestBodyPost} | |||
*/ | |||
public List<MgmtDistributionSetRequestBodyPost> buildAsList(final int offset, final int count) { | |||
final ArrayList<MgmtDistributionSetRequestBodyPost> bodyList = Lists.newArrayList(); |
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.
Interface
@@ -240,7 +239,9 @@ private void updateActionStatus(final Message message) { | |||
final Action action = checkActionExist(message, actionUpdateStatus); | |||
|
|||
final List<String> messages = actionUpdateStatus.getMessage(); | |||
if (ArrayUtils.isNotEmpty(message.getMessageProperties().getCorrelationId())) { | |||
|
|||
if (message.getMessageProperties().getCorrelationId() != 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.
Extract into method
@@ -8,7 +8,6 @@ | |||
*/ | |||
package org.eclipse.hawkbit.util; | |||
|
|||
import static com.google.common.net.HttpHeaders.X_FORWARDED_FOR; |
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.
keep guava but on text scope
Signed-off-by: kaizimmerm <[email protected]>
CONTRIBUTING.md
Outdated
* use [guava](https://github.com/google/guava) if feasible | ||
* use [Apache commons lang](https://commons.apache.org/proper/commons-lang/) if feasible | ||
|
||
Note that the guava project for instance often documents where they thing that JDK is having a similar functionality (e.g. their thoughts on [Throwables.propagate](https://github.com/google/guava/wiki/Why-we-deprecated-Throwables.propagate)). |
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.
thing? Do you mean think?
assertEquals(Lists.newArrayList( | ||
new ArtifactUrl("http".toUpperCase(), "download-http", HTTP_LOCALHOST + TENANT + "/controller/v1/" | ||
+ CONTROLLER_ID + "/softwaremodules/" + SOFTWAREMODULEID + "/artifacts/" + FILENAME)), | ||
assertEquals( |
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.
You use org.assertj.core.api.Assertions.assertThat and org.junit.Assert.assertEquals in this TestClass. I think it would be great to use the same Assert class where it is possible and avoid using different Assert classes.
E.g. in this case you could use
assertThat( Arrays.asList(new ArtifactUrl("http".toUpperCase(), "download-http", HTTP_LOCALHOST + TENANT + "/controller/v1/" + CONTROLLER_ID + "/softwaremodules/" + SOFTWAREMODULEID + "/artifacts/" + FILENAME_ENCODE))) .isEqualTo(ddiUrls);
This could be done in another pull request later on.
@@ -199,7 +198,7 @@ public static Panel getDistributionSetInfo(final DistributionSet distributionSet | |||
* @return Label | |||
*/ | |||
public static Label createNameValueLabel(final String label, final String... values) { | |||
final String valueStr = StringUtils.join(Arrays.asList(values), " "); | |||
final String valueStr = StringUtils.arrayToCommaDelimitedString(values); |
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.
Now the String is comma separated. The original was separated by a space character.
@@ -39,7 +39,7 @@ | |||
public class MgmtDownloadArtifactResource implements MgmtDownloadArtifactRestApi { | |||
|
|||
@Autowired | |||
private SoftwareManagement softwareManagement; | |||
private SoftwareModuleManagement softwareManagement; |
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 ´softwareManagement´ to ´softwareModuleManagement´
@@ -82,7 +82,7 @@ | |||
private ControllerManagement controllerManagement; | |||
|
|||
@Autowired | |||
private SoftwareManagement softwareManagement; | |||
private SoftwareModuleManagement softwareManagement; |
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 softwareManagement
to softwareModuleManagement
@@ -63,7 +63,7 @@ | |||
private static final Logger LOG = LoggerFactory.getLogger(MgmtDistributionSetResource.class); | |||
|
|||
@Autowired | |||
private SoftwareManagement softwareManagement; | |||
private SoftwareModuleManagement softwareManagement; |
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 softwareManagement
@@ -39,7 +39,7 @@ | |||
public class MgmtDownloadArtifactResource implements MgmtDownloadArtifactRestApi { | |||
|
|||
@Autowired | |||
private SoftwareManagement softwareManagement; | |||
private SoftwareModuleManagement softwareManagement; |
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 softwareManagement
@@ -57,7 +57,7 @@ | |||
private ArtifactManagement artifactManagement; | |||
|
|||
@Autowired | |||
private SoftwareManagement softwareManagement; | |||
private SoftwareModuleManagement softwareManagement; |
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 softwareManagement
@@ -21,9 +21,9 @@ | |||
*/ | |||
public class JpaDistributionSetTypeBuilder implements DistributionSetTypeBuilder { | |||
|
|||
private final SoftwareManagement softwareManagement; | |||
private final SoftwareModuleManagement softwareManagement; |
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 softwareManagement
@@ -26,9 +26,9 @@ | |||
public class JpaDistributionSetTypeCreate extends AbstractDistributionSetTypeUpdateCreate<DistributionSetTypeCreate> | |||
implements DistributionSetTypeCreate { | |||
|
|||
private final SoftwareManagement softwareManagement; | |||
private final SoftwareModuleManagement softwareManagement; |
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 softwareManagement
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
Signed-off-by: kaizimmerm <[email protected]>
…sage (eclipse-hawkbit#524) * Split DS management and reduce util usage. Signed-off-by: kaizimmerm <[email protected]> * Split sw module and type management. Signed-off-by: kaizimmerm <[email protected]> * Sonar issues. Signed-off-by: kaizimmerm <[email protected]> * Make sonar listen to the exception! Signed-off-by: kaizimmerm <[email protected]> * Register both beans. Signed-off-by: kaizimmerm <[email protected]> * Split JPA implementations. Signed-off-by: kaizimmerm <[email protected]> * Revert user details change. Signed-off-by: kaizimmerm <[email protected]> * Fix compilation errors. Signed-off-by: kaizimmerm <[email protected]> * Fix bean queries. Fix image path. Signed-off-by: kaizimmerm <[email protected]> * Document preferred utility usage. Signed-off-by: kaizimmerm <[email protected]> * Fix exmaples and revert unintended checkin. Signed-off-by: kaizimmerm <[email protected]> * Code cleanup. Signed-off-by: kaizimmerm <[email protected]> * Typos, readibility. Signed-off-by: kaizimmerm <[email protected]> * Remove unused reference. Signed-off-by: kaizimmerm <[email protected]> * Rollouts cache delete aware. Signed-off-by: kaizimmerm <[email protected]> * Fix rolloutgroup delete event. Signed-off-by: kaizimmerm <[email protected]> * Add new RolloutGroupDeletedEvent event Signed-off-by: kaizimmerm <[email protected]>
One more step in repository refactoring (see #197). I split software and distribution set management for type and instance management (more splits will follow e.g. TagManagement).
Another topic that is constantly bugging many hawkBit developers is the chaos we introduced with using a wide array of utility libraries. You can basically see in code which dev prefers apache or spring utils or Apache commons or JDK stuff.
This cannot be fixed over night. But I documented in the contributors guide a recommended priority and refactored the code base by means of the "low hanging fruits" to implement the new guideline.
I think in the long run we should be able to get rid of Apache commons and reduce guava to use cases where it actually makes sense. Spring utils imho can be used freely, hawkBit is based on Spring and will continue to do so.
Let me know what you think.
Reviewers: @sirwayne @melanieretter