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

feat: jackrabbit package plugin #489

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

friendlymahi
Copy link

@friendlymahi friendlymahi commented Sep 22, 2023

Fixes #488

Proposed Changes

  • Content Package Plugin version is too old
  • Packaging is replaced with Jackrabbit Package Plugin
  • Disabled validation just on filter path as it is dependent on consumers using the package

Screenshots and Logs

Before

Error mentioned in #488

After

Build went through fine without any issues

Background context

As per Adobe's documentation Content Package Maven Plugin is no longer supported, and needs replacement with Jackrabbit Package Maven Plugin.

How should this be manually tested?

Create a new AEM project with react as frontend module type. Use the command npm pack in the code obtained from this PR, and place it under node_modules folder of ui.frontend. Then update the npm build script to react-scripts build && clientlib && aem-packager

Questions:

  • Does this add new dependencies (node, maven, ant, etc) that must be installed?
    • No
  • Does this change the commands required to run this project?
    • No
  • Are any environmental dependencies or operational support steps (sql commands, config changes, etc) required?
    • No
  • Are there any relevant issues besides this one?
    • No

- Content Package Plugin version is too old
- Replacing its usage with Jackrabbit Package Plugin
- Disabled validation to ensure maven build doesnt throw errors

Fixes amclin#488
@friendlymahi
Copy link
Author

@amclin - Requesting you to review this issue and PR, and if everything looks good, please merge the PR. Thanks !

@friendlymahi
Copy link
Author

@amclin - hope you are doing well. can you please review this PR as I would like to use this package for our teams where aem or Java development is not required. If not feasible please let me know so I can copy over the code to our internal module and remove it once you merge the PR. Thanks

@amclin
Copy link
Owner

amclin commented Oct 7, 2023

@friendlymahi this is a breaking change - it breaks the ability to continue using CRX package deployments

Can you try including both plugins, so that CRX package deployment can still be supported without causing a breaking change?
https://jackrabbit.apache.org/filevault-package-maven-plugin/migrating.html

Also, please reenable validation. If maven is throwing errors, this project should not be suppressing them.

@friendlymahi
Copy link
Author

friendlymahi commented Oct 7, 2023

@amclin I tested this change and it works fine. Let me explain a bit.

  1. Regarding multiple pluguns - Existing plug-in is failing to locate some classes and as a result the zip file is not created. And we can't have both plug-ins doing the same task. So the build will fail. One option I can think of is, creating an additional maven profile and letting the users provide maven profile also in the configuration if they want to use the new plug in. But regardless I will include both and see what happens and post an update.
  2. Regarding validator settings - Typically in a regular aem maven project we provide repository structure stating what roots are allowed. But here since pom file is coming from this library we can't provide it unless we are ready to make addl code changes. And that too the repository structure or the filter root generation logic should be dependent on the name or npm package name attribute. Now we know that this npm module only generates an independent package without having entire maven project. So validating it's structure and failing the build process is not reasonable. However a warning provides some information to the developer consuming this. Also this strict structure validation is not available in old version of the maven plug in.

Please let me know what you think.

Thanks

@amclin
Copy link
Owner

amclin commented Oct 7, 2023

Existing plug-in is failing to locate some classes and as a result the zip file is not created.

@friendlymahi this is something specific to your corporate environment, and not a result of using the content-package-maven-plugin. The plugin works just fine with Maven 3.9.3 and OpenJDK 21, as well as far back with the older versions of Java and Maven that aem-packager was originally built with. I don't know what your corporate environment is doing that is blocking the plugin's dependencies, as I said, I recall seeing something in the past about weirdness with expired SSL certs on Adobe's registries that caused issues with certain versions of Java, but ultimately that issue isn't within the scope of aem-packager to be able to solve, and making a breaking change for all consumers to work around your particular corporate environment limitation certainly isn't good practice.

That said, I am open to adding Jackrabbit for the benefits it brings, and making a breaking change for that. I have an idea about how to handle this in a way that gives consumers who need CRX Deployments the ability to continue doing so. Let me see what I can put together for this. I may need your help to test since I don't currently have access to an AEM instance to work with.

@friendlymahi
Copy link
Author

sure I can work with you on this. Here is what I did. I used Adobe's latest maven plugin which is v1.0.4. When doing so the zip does get created. However the filter.xml is missing in it. In other words the package becomes unusable for installing via CRX Package Manager. I tested this on my personal Mac which is not on any corp network. And I understand that probably because you dont have a local AEM instance you cant validate the package. I havent tried this, but see if this alternative helps without AEM in place - https://github.com/apache/sling-org-apache-sling-app-cms?tab=readme-ov-file.

One part that I am unclear is consumers who need CRX Deployments the ability to continue doing so, the npm package only asists with creation of zip but not install on a target AEM instance even though the Adobe's plugin is capable of it i.e. the npm package doesnt take any params to support auto deployment using addl maven params.

- Content Package Plugin version is bumped and not removed completely
- Jackrabbit Package Plugin:
   - Updated validator settings
   - Enabled validation, just excluded filter path validation
@friendlymahi
Copy link
Author

@amclin - Made my final changes including the content package maven plugin with latest version. Validations are now just excluded for jcrPath since that is unique for every application and cant be assumed without configuring through one of our available parameters or a new one. So made it a warn instead of error. This should keep it simple and minimal. If everything looks good, please merge the PR. Else let me know for more changes. Thanks !

Switches out the deprecated JCR Valut plugin
for content packages with the newer Jackrabbit.

Should speed up package installations, and reduce
the amount network or security errors around using
an old unmaintained plugin.

BREAKING CHANGE: no longer contains the necessary
maven plugins for CRX deployment functionalitty. To add legacy CRX support on top of Jackrabbit, set the new option `legacyCRXSupport` to true. To  restore the old `aem-packager` functionality entirely, set the new
option `packager` to `jcrvault`
@amclin
Copy link
Owner

amclin commented Oct 8, 2023

Thanks @friendlymahi, the warn-based approach makes sense.

Can you take a look at this branch please:
https://github.com/amclin/aem-packager/tree/feat/jackrabbit

By default it does what you are looking for. But also, I would appreciate if you can test the new option "legacyCRXSupport": true which creates the version with the jar value added for CRX support.

If both of those are functioning, then I'll get this merged in. The approach I'm using here will let me fix some other bugs that have been bothering me.

@amclin amclin mentioned this pull request Oct 8, 2023
@amclin amclin added the dependencies Pull requests that update a dependency file label Oct 8, 2023
Mahidhar Chaluvadi added 2 commits October 8, 2023 00:01
 into feat-filevault-pkg-plugin

# Conflicts:
#	src/templates/pom.xml
@friendlymahi
Copy link
Author

@amclin - Pulled your changes to my branch and validated the same. Variable mappings were missing, and I fixed them too. Please take a look at the last commit I did. Validated all the 3 usecases (default, jcrvault, jackrabbit+crxCompatibility), and pacjkages are created as expected. However I have no clue on the usage of content-package-maven-plugin when we are not installing to an AEM instance. So except for that all other use cases are validated. If all good, please merge the PR, and this shall automatically merge your PR as well I guess. Thanks !

@friendlymahi
Copy link
Author

@amclin - Are the changes good? Looking for new version so I can use this at my work for a demo. Thanks !

@friendlymahi
Copy link
Author

@amclin - Any luck with the review? Thanks

@friendlymahi
Copy link
Author

@amclin - Hope you are doing well. Can you please help moving this PR if everything is okay? We recently upgraded our AEM environment and want to validate the module against the new version of AEM we have. Thanks!

@friendlymahi
Copy link
Author

Hello @amclin

Hope you are doing well.

Can you please confirm please review and merge this PR? if not feasible, I will fork the code and go with my custom solution as this is long overdue for my work.

Thanks in advance,
Mahidhar Ch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class not found exception when using plugin
2 participants