Skip to content

Conversation

@MikeThomsen
Copy link
Contributor

Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
  • Have you written or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
  • If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

@MikeThomsen
Copy link
Contributor Author

@joewitt In 2.5.0 they removed the groovy-all fat jar in favor of doing a pom import that brings in all of the individual groovy components. Should I change the NOTICE to say "groovy-all pom and associated Groovy modules" (where it currently reads something like "groovy-all 2.4.X jar") or should I remove that completely in favor of enumerating each Groovy component?

@mattyb149
Copy link
Contributor

Unfortunately I believe you need to enumerate all the transitive dependencies in the L&N. You should have been able to copy that from the Groovy NOTICE, but it does not have its own dependencies listed in there (assuming some of the dependencies have NOTICEs), so we have to do it here. Their LICENSE mentions a couple of dependencies, but I'm not sure it's complete, so you'll have to verify both our L&N include the appropriate entries.

@MikeThomsen
Copy link
Contributor Author

Thanks, that's what I thought, but need a second opinion because we gotta cross our Ts and dot our Is on this stuff. Will fix.

@MikeThomsen
Copy link
Contributor Author

@mattyb149 @joewitt while I'm doing L&N validation on their dependencies, I can also cut out some of the fat by dropping Groovy modules like the CLI one that have no bearing on NiFi scripting. Thoughts?

@MikeThomsen
Copy link
Contributor Author

MikeThomsen commented Oct 3, 2018

@mattyb149 @joewitt Removed the following as compile dependencies because I can't see any valid use for them in our scripting scenarios:

  • groovy-cli*
  • groovy-groovydoc
  • groovy-groovysh
  • groovy-docgenerator
  • groovy-swing
  • groovy-test
  • groovy-test-junit5
  • groovy-testng

Think that's it. We might want to release an unofficial test build of NiFi w/ 2.5.X because I'm not 100% sure there's a full guarantee of compatibility between 2.4 and 2.5

@MikeThomsen
Copy link
Contributor Author

@mattyb149 Next major release of Groovy will be 3.0. This is 2.5.X, which AFAIK, is not a breaking release from 2.4.X. If we wait until "NiFi 2.0," I think it's likely that we're going to end up making it even harder for people to migrate. At the very least, I think we need to offer this as a profile build option.

@jtstorck
Copy link
Contributor

jtstorck commented Jun 1, 2019

@MikeThomsen We should collaborate on this Groovy upgrade. In my WIP pull request for Java 11 build compatibility, I also upgraded Groovy. I also tried to clean up the groovy dependencies by having each NiFi module depend on the specific Groovy libs needed, rather than using groovy-all.

I am in the process of splitting out the changes in PR 3404 into smaller PRs, and the groovy upgrade would be one of them. Could you take a look at the groovy changes I made in PR 3404? You've probably tested your PR more than I have tested mine; I was focusing on getting the build (compilation and tests) working on Java 11, and getting the NiFi UI to come up on Java 11, but I didn't do much testing of processors that use Groovy.

@MikeThomsen
Copy link
Contributor Author

Sure. My thinking with my PR was "for tests, it doesn't really matter" because it adds no additional weight to the build and for groovyx and the scripting bundle there were only a few things in groovy-all that I thought should be excluded. What are your thoughts there?

@jtstorck
Copy link
Contributor

jtstorck commented Jun 4, 2019

@MikeThomsen
I haven't gotten to the point where I am extracting the Groovy update from PR 3404. I need to put up a PR for Mockito 2.x and PowerMock 2.x, and then I was going to put up a PR for the Groovy upgrade. If you're okay holding this PR open for a few more days, we can compare notes and try to pull all the Groovy changes together.

I'd like to see us avoid, and I'm not sure if this is the proper term, using "aggregate" poms like groovy-all, and instead reference specific modules needed as dependencies whether for production or test code. In addition, I'd like to keep moving toward parent poms only establishing dependency management, rather than forcing dependencies on the submodules. I did a bit of that in PR 3404.

@MikeThomsen
Copy link
Contributor Author

Yeah, I'm fine with keeping it open. We can even do this as a PR onto your Java 11 branch if you want.

@jtstorck
Copy link
Contributor

jtstorck commented Jun 4, 2019

The scope of PR 3404 is too large to expect good review, so it's basically just a placeholder PR until all the dependency upgrades are done. A few PRs have been extracted from it so far, and after the nifi-metrics module and Mockito/PowerMock PRs are created and merged to master, I'll be rebasing the Java 11 branch against master and trimming out the extracted parts. Eventually it'll be a much smaller change-set that enables the build to be done on Java 11, but we're still a way off of that. I'm trying to pull things out into specific change-sets so there's a better record of what was needed for Java 11, and referencing all the JIRAs in NIFI-5176.

@MikeThomsen
Copy link
Contributor Author

@jtstorck I'm going to try to pare down the use of groovy-all.

@jtstorck
Copy link
Contributor

jtstorck commented Jun 21, 2019

@MikeThomsen I'll be pushing a rebased PR 3404 fairly soon, which already includes replacing groovy-all with all the individual modules, and adds the needed modules to each lowest-level POM. It is that way on the current PR, if you want to take a look, though some of the changes are spread across a few commits, mainly this one.

Right now, I'm working on fixing some tests on my local rebase, and currently have the groovy dependency version set to 2.5.7.

NIFI-5254 Updated NOTICE files with full list of Groovy modules now that groovy-all fat jar has been split up.
NIFI-5254 Removed Groovy from NOTICE in record-serialization-services-nar because mvn dependency:tree showed all groovy references to be for test scope.
NIFI-5254 Cleaned up a bunch of (probably) unneeded Groovy modules that were split out from the fat jar in 2.5.X
NIFI-5254 Updated L&N in groovyx bundle and updated all of the Groovy dependencies to have the same exclusions that were used in the scripting bundle.
NIFI-5254 Updated L&N and dependencies for NiFi Toolkit.
NIFI-5254 Updated assembly L&N for new Groovy changes.
NIFI-5254 Updated to Groovy 2.5.5
NIFI-5254 Updated to groovy 2.5.6.
NIFI-5254 Updated to Groovy 2.5.7.
@MikeThomsen
Copy link
Contributor Author

@jtstorck I spent a lot of time last night going down a rabbit hole with nifi-toolkit trying to break things up, upgrade Spock, etc. I'm leaving this one as-is for now.

@MikeThomsen MikeThomsen closed this Aug 7, 2019
@MikeThomsen MikeThomsen deleted the NIFI-5254 branch August 7, 2019 01:04
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.

3 participants