Dependency management setup#2876
Conversation
|
Can one of the admins verify this patch? |
JonathanGiles
left a comment
JonathanGiles
left a comment
There was a problem hiding this comment.
Looks good to me. There are follow-on clean-ups and tweaks we should do (especially ensuring all the build plugins are required, tidying up some descriptions, etc), but nothing to block merging in my opinion.
| <!-- Dependency Versions --> | ||
| <jackson.version>2.9.6</jackson.version> | ||
| <azure-client-runtime.version>1.3.0</azure-client-runtime.version> | ||
| <client-runtime.version>1.6.1</client-runtime.version> |
There was a problem hiding this comment.
I wonder how this line and the azure-client-runtime version do not conflict; ideally they should be same or close...
There was a problem hiding this comment.
This is a good point. I would also love to see us bring some of these dependencies up to date - 1.3.0 is a year old at this point, and as we bring in track one SDK components, we should try to get them moving towards the more recent releases, e.g. 1.6.5.
|
|
||
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> |
There was a problem hiding this comment.
You removed azure-compiler-plugin from azure-keyvault-extensions, but left here, is there any specific reason for that?
There was a problem hiding this comment.
removed it.
Thanks for spotting it.
| - bash: | | ||
| LOG_PARAMS='-Dorg.slf4j.simpleLogger.defaultLogLevel=error -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn' | ||
| mvn test -Dhttp.keepAlive=false -Dsurefire.rerunFailingTestsCount=3 $LOG_PARAMS -f pom.client.build.xml | ||
| mvn test -Dhttp.keepAlive=false -Dsurefire.rerunFailingTestsCount=3 $LOG_PARAMS -f pom.client.xml |
There was a problem hiding this comment.
What about the client.yml? Is the plan to update the pipeline variable after this merges? We should consider adding a variable for this project file as well.
There was a problem hiding this comment.
updated the client.test.live.yml file to have pomFile variable.
| <relativePath>../pom.xml</relativePath> | ||
| </parent> | ||
|
|
||
| <groupId>com.microsoft.azure</groupId> |
There was a problem hiding this comment.
Isn't this defaulted from the parent POM? Is there any reason to duplicated in the individual projects?
There was a problem hiding this comment.
We should default the other properties like versioning and developer, etc in the parent POM as well to avoid needing them in all the individual projects.
There was a problem hiding this comment.
The group Ids were introduced to improve readability. Other properties already existed.
Helps to add dependency for the project, without looking at other poms for info about group Ids or versions.
There was a problem hiding this comment.
It might help with some local views of the project but it adds more harm then good in my opinion as it enables more projects to stay in sync, in particular for the version number, the fewer places we have to edit that the better. So I still suggest that a number of common properties should be lifted into one of the parent POM's in the structure. Of course that can happen as a clean-up PR and not block this PR.
There was a problem hiding this comment.
I'm fine to centralise versions where it makes sense. I would rather not centralise group ID for now though. I would say this should be done subsequently so that it does not hold merging. We should get this merged ASAP
|
Looks like our DevOps build job is failing https://dev.azure.com/azure-sdk/public/_build/results?buildId=3174, although it isn't reporting as failing. I filed issue #2880 to investigate why the job isn't correctly being marked as failed but please investigate the failures to get them addressed. |
| @@ -1,19 +1,14 @@ | |||
| <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |||
There was a problem hiding this comment.
I assume the plan is to follow-up with another PR later to remove this file once you can update the azure pipeline to point to the new project file.
There was a problem hiding this comment.
yeah, once the migration is successfully done. The plan is to delete this file.
|
@JonathanGiles a number of errors are showing up https://dev.azure.com/azure-sdk/public/_build/results?buildId=3173 but I also see "2019-01-25T02:45:29.1847285Z [WARNING] checkstyle:check violations detected but failOnViolation set to false". Is the failOnViolation controlled by one of these parent POMs? |
Replying to this. |
I don't like having ignored errors showing up in our logs it will do nothing but cause noise and confusion. We should fix those ASAP or disable the rules that are now failing that weren't before. |
|
The plan is to identify the issues that checkstyle identifies and work to resolve them all with urgency. We want to fail on warning as soon as we are clean. The problem right now is that the repo has added code before we enabled the checks, so we are on the backfoot. Everyone is on the same page here. |
JonathanGiles
added
the
Client
6 participants
Introduces new pom structure for parent and all children pom including keyvault and its sub modules.
Single parent pom is created (pom.client.xml).
Yml files are updated to use pom.client.xml
Keyvault and its children modules are cleaned up and restructured.
Parent Pom (pom.client.xml) takes dependency on Microsoft wide Java Parent pom: (https://github.com/Microsoft/maven-java-parent)
Introduces, spotbugs, checkstyle and javadoc checks from Microsoft wide Java parent pom.
Cleans up unneeded libraries and plugins from main and submodules.
Other changes:
Keyvault cryptography tests resources reading was fixed.