Skip to content

NIFI-10588 Add a new goal that checks dependency duplications in nars#6418

Closed
SaumyaGurtu wants to merge 2 commits intoapache:mainfrom
SaumyaGurtu:nifi-10457
Closed

NIFI-10588 Add a new goal that checks dependency duplications in nars#6418
SaumyaGurtu wants to merge 2 commits intoapache:mainfrom
SaumyaGurtu:nifi-10457

Conversation

@SaumyaGurtu
Copy link
Contributor

@SaumyaGurtu SaumyaGurtu commented Sep 14, 2022

Summary

NIFI-10588

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.
locally tested with mvn verify

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 8
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@MikeThomsen MikeThomsen left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but we need to try to get to 5/5 passes on the CI/CD pipelines before merging something like this.

pom.xml Outdated
<version>3.1.0</version>
<executions>
<execution>
<id>no-duplicate-declared-dependencies</id>
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me wonder if we shouldn't make plans to ban dependency convergence to encourage more discipline on dependency management.

@exceptionfactory
Copy link
Contributor

@MikeThomsen and @SaumyaGurtu I'm not sure this PR accomplishes the goal described in NIFI-10457. Although the plugin configuration checks for duplicate dependencies, it does not highlight when certain dependencies should be marked as provided. Therefore, I'm not sure this should be merged.

@SaumyaGurtu
Copy link
Contributor Author

@MikeThomsen The proposed solution has passed the checks and highlights the duplicate dependencies in the project if any as asked in the ticket. This will increase the discipline in the repository.
Please let me know if you have any change in plans.

@exceptionfactory
Copy link
Contributor

@SaumyaGurtu Although this pull request looks like a useful improvement, it does not accomplish the goals described in the associated Jira issue. The goal described in the Jira issue is specifically related to NAR bundles. Although a NAR bundle may not contain direct duplication of dependencies, the hierarchical nature of NAR dependencies means that a child NAR could have the same library as a parent NAR, making the extra dependency unnecessary in the child NAR. This kind of relationship is not necessarily detected through the maven-enforcer-plugin because NAR bundles are specific to Apache NiFi. For this reason, an alternative approach will be necessary to accomplish the goal described in the Jira issue.

@exceptionfactory exceptionfactory changed the title NIFI-10457: Add a new goal that checks dependency duplications in nars NIFI-10588 Add a new goal that checks dependency duplications in nars Oct 7, 2022
@exceptionfactory
Copy link
Contributor

exceptionfactory commented Oct 7, 2022

@SaumyaGurtu, although this PR does not align with the goals of NIFI-10457, it is still a useful improvement and it does appear to implement the features described in NIFI-10588. I updated the title and link to relate to that Jira issue.

With that change, this can be reviewed with the purpose of banning duplicate dependencies. This needs to be verified in a local build to ensure that it works as designed.

@dan-s1
Copy link
Contributor

dan-s1 commented Oct 7, 2022

@exceptionfactory I think you probably meant NIFI-10457 not NIFI-10467

@exceptionfactory
Copy link
Contributor

@exceptionfactory I think you probably meant NIFI-10457 not NIFI-10467

Thanks for the correction @dan-s1!

@dan-s1
Copy link
Contributor

dan-s1 commented Oct 7, 2022

@exceptionfactory I am not sure how this differs from what I tried for NIFI-10565 but could not get working with the Maven Enforcer plugin. If this really worked it should have caught the issues I corrected in NIFI-10565.

@exceptionfactory
Copy link
Contributor

@dan-s1 Yes, it sounds similar. This may only check for duplicates declared in the pom.xml itself, without reference to dependencies inherited from parent definitions. With your previous testing, that would be a key point to evaluate, and I plan to take a closer look soon. This warrants some reading of the Maven Enforcer Plugin documentation.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Although this rule does not appear to block duplicate dependencies from a parent module, it is still a helpful improvement to avoid internal duplicates in the same pom.xml. I noted one adjustment where the new rule should be moved to the current execution section, and then this should be ready.

pom.xml Outdated
</goals>
<configuration>
<rules>
<banDuplicatePomDependencyVersions/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding a new execution block, the banDuplicatePomDependencyVersions rule should be added to the existing execution section.

@exceptionfactory
Copy link
Contributor

@SaumyaGurtu, are you able to make the adjustments suggested to move the banDuplicatePomDependencyVersions under the current Maven Enforcer configuration?

@exceptionfactory
Copy link
Contributor

@SaumyaGurtu Based on the work you already did, I updated the branch to move the ban rule under the current set of enforcer rules. I also rebased the PR so that we can get a current status of the build with this rule enabled.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution @SaumyaGurtu. Although this PR changed directions, it is still a useful step forward in managing project dependencies. +1 merging

priyanka-28 pushed a commit to priyanka-28/nifi that referenced this pull request Nov 16, 2022
This closes apache#6418

Signed-off-by: David Handermann <exceptionfactory@apache.org>
lizhizhou pushed a commit to lizhizhou/nifi that referenced this pull request Jan 2, 2023
This closes apache#6418

Signed-off-by: David Handermann <exceptionfactory@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants