-
Notifications
You must be signed in to change notification settings - Fork 4
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
base: master
Are you sure you want to change the base?
feat: jackrabbit package plugin #489
Conversation
- 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
@amclin - Requesting you to review this issue and PR, and if everything looks good, please merge the PR. Thanks ! |
@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 |
@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? Also, please reenable validation. If maven is throwing errors, this project should not be suppressing them. |
@amclin I tested this change and it works fine. Let me explain a bit.
Please let me know what you think. Thanks |
@friendlymahi this is something specific to your corporate environment, and not a result of using the 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. |
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 |
- Content Package Plugin version is bumped and not removed completely - Jackrabbit Package Plugin: - Updated validator settings - Enabled validation, just excluded filter path validation
@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`
Thanks @friendlymahi, the warn-based approach makes sense. Can you take a look at this branch please: By default it does what you are looking for. But also, I would appreciate if you can test the new option 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 - 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 ! |
@amclin - Are the changes good? Looking for new version so I can use this at my work for a demo. Thanks ! |
@amclin - Any luck with the review? Thanks |
@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! |
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, |
Fixes #488
Proposed Changes
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 toreact-scripts build && clientlib && aem-packager
Questions: