Skip to content
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

chore: Clean up dhis-api and other modules using it [TECH-801] #9302

Merged
merged 11 commits into from
Nov 25, 2021
Merged

Conversation

enricocolasante
Copy link
Contributor

@enricocolasante enricocolasante commented Nov 24, 2021

dependency-graph

The idea of this PR is to start from the bottom of the dependency tree and clean up the unused declared and the used undeclared dependencies in the pom.
mvn dependency:analyze-only is not trustable 100% so what I am doing is to exclude all the attached dependencies from the target module. In this case, something like this:
<dependency> <groupId>org.hisp.dhis</groupId> <artifactId>dhis-api</artifactId> <exclusions> <exclusion> <groupId>*</groupId> <artifactId>*</artifactId> </exclusion> </exclusions> </dependency>

Then fixing all the dependency problems.

In this PR there was also a problem in dhis-web-api-test tests because of the import of simlper logger from rule-engine. I exclude that from the dependency to make it work. Maybe this will need some more thought that are not part of this PR.

@enricocolasante enricocolasante added the run-api-tests This label will trigger an api-test job for the PR. label Nov 24, 2021
Copy link
Contributor

@teleivo teleivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome approach @enricocolasante 😄

regarding logging you are right there is some more work that we need to do at some point: https://jira.dhis2.org/browse/TECH-803

I also found that some of our code logs using simplelogger #9176 (quick fix to quiet these logs during testing). So I would wait merging this PR after dhis2/dhis2-rule-engine#81 is merged. Since you removed the simplelogger config tests would get noisy again.

What do you think of enforcing/banning simplelogger from being included via
https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html It can also ban it if coming via transitive dependency 😋
?

@enricocolasante
Copy link
Contributor Author

enricocolasante commented Nov 24, 2021

awesome approach @enricocolasante 😄

regarding logging you are right there is some more work that we need to do at some point: https://jira.dhis2.org/browse/TECH-803

I also found that some of our code logs using simplelogger #9176 (quick fix to quiet these logs during testing). So I would wait merging this PR after dhis2/dhis2-rule-engine#81 is merged. Since you removed the simplelogger config tests would get noisy again.

I removed simplelogger dependency from the system, so the file is no longer required.
Now log should follow the rules declared in the log4j files.

What do you think of enforcing/banning simplelogger from being included via https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html It can also ban it if coming via transitive dependency 😋 ?

We can ban it, but maybe for now it is an overkill. It was imported just from rule-engine, removing it from there should be enough.

And yes, the change of rule-engine version was not meant to be part of the PR. I rebased and your comment disappeared.

@teleivo
Copy link
Contributor

teleivo commented Nov 24, 2021

We can ban it, but maybe for now it is an overkill. It was imported just from rule-engine, removing it from there should be enough.

Every project I worked on that ran on a JVM had messed up logging 😂 😡 😅 Someone just needs to add another dependency and we are back at where we were. Thats why I vote for getting the enforcer in 🥺 I am happy to do it if this issue is already draining. It's a tough, tedious one 😅

Make sense.
I banned it ❌
And it works also for transitive dependencies. Removing the exclude from rule-engine dependency makes the build fail

@teleivo
Copy link
Contributor

teleivo commented Nov 24, 2021

@enricocolasante weird it looks like you edited my comment #9302 (comment) 😂

Copy link
Contributor

@teleivo teleivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you very much for the enforcer change 😄
nice work!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@teleivo teleivo merged commit 13e9f9f into master Nov 25, 2021
@teleivo teleivo deleted the TECH-801 branch November 25, 2021 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-api-tests This label will trigger an api-test job for the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants