Adobe-Consulting-Services / acs-aem-commons

http://adobe-consulting-services.github.io/acs-aem-commons/
Apache License 2.0
451 stars 597 forks source link

Split application content from mutable content #1975

Closed DominikSuess closed 4 years ago

DominikSuess commented 5 years ago

Required Information

Expected Behavior

As showcased in the AEM Archetype and following the typification of JCRVLT-170 packages containing application content (libs & apps) should be split from remaining content. This is in preparation for scenarios using composite node store where the application part can and must be installed during deployment time while content accounting to the mutable part can only be installed after startup (at runtime)

It is fine to nest both packages in a shared container for convenience but it must be possible to declare dependencies on the individual parts (following changes in JCRVLT-336 you can expect mixed content packages to be installed as application packages while the rest of the content would be ignored.

Actual Behavioral

Acs commons is a single mixed content package.

Steps to Reproduce

Check sources.

Links

Links to related assets, e.g. content packages containing test components

DominikSuess commented 5 years ago

@davidjgonzalez & @badvision please also crosscheck the content being located in etc which is no longer the recommended location. For AEM we relocated these things but established mechanisms to stay backward compatible if content still resides in the old areas. - this is specifically true for content that has rather application characteristic and would be better located under apps (e.g. clientlibs).

badvision commented 5 years ago

We have to maintain backwards compatibility to 6.3, so whatever we do has to work there as well.

badvision commented 5 years ago

To be clear, there is a content folder in the package but it's just an ACL for DAM to support the acs-commons-review-task-asset-mover-service. Is there any other application content you're seeing that needs to be relocated other than the things in /etc?

davidjgonzalez commented 5 years ago

FYI - im going to time box this and see how far i get

davidjgonzalez commented 5 years ago

Proposed project structure:

The content project package, which will deploy using the same name package name/versioning (allowing for backwards compat) will now contain:

Here's a high-level visual representation of the proposed break-out:

image

badvision commented 5 years ago

FWIW I think we can move the /etc content type stuff (anything that renders a page, etc) into /apps/acs-commons/content and change paths to point to /mnt/override/acs-commons/content ...

This is pretty similar to how things like pickers and data sources already work in the product with /lib/cq/content, etc

davidjgonzalez commented 5 years ago

I broke out everything in /etc more clearly; i think quite a bit could be moved out of /etc into /apps per https://helpx.adobe.com/experience-manager/6-4/sites/deploying/using/repository-restructuring.html

davidjgonzalez commented 5 years ago

Question to the group, can we change the "content" projects name and still keep it generally backwards compatible? Ideally, we have an "all" or "container" and the "ui.content" and "ui.apps"

An option, though I worry it might be confusing is we could rename all the maven projects but have the final build artifacts (jar file and deployable content package) names remain the same.

Edit: talked it through with @DominikSuess offline; and as long as the mutable project has the same package ID as our current "content" project, we should be good.

So we could have

IDK if we want to diverge project names from package names and have:

Or something ... seems like we should try to keep everything consisten. i dont think well end up not aligning to the AEM Maven Archetype convetions, but i guess we should determine how far and when/where we diverge. Nows the time to align if we want to.

justinedelson commented 5 years ago

Ideally when updating versions, a consumer would just update the version part of the coordinates in their pom (I think @DominikSuess is talking about this from a package manager standpoint, which is fair, but a different question IMO). That's what we've "trained" consumers to do and it seems confusing to change at this point.

So my suggestion would be to keep acs-aem-commons-content as the Maven artifactId for the container and have that embed acs-aem-commons-ui.apps and acs-aem-commons-ui.content at least for the rest of 4.x. IMO changing that to acs-aem-commons-all is a 5.0 thing.

davidjgonzalez commented 5 years ago

@justinedelson good point

davidjgonzalez commented 5 years ago

Few questions that have come up during re-org.

justinedelson commented 5 years ago

@davidjgonzalez Unfortunately, we need min for everything since there are twitter paths in both /apps and /etc :(

davidjgonzalez commented 5 years ago

@justinedelson TBH I'm not sure how to aggregate ui.apps min and ui.content min into content min package. Any chance you could whip that up in a PR to https://github.com/davidjgonzalez/acs-aem-commons/tree/feature/1975 ?

davidjgonzalez commented 5 years ago

@adamcin any thoughts on if/how we should split oak-pal checks for ui.apps vs ui.content?

adamcin commented 5 years ago

@davidjgonzalez I think the existing checklists can (and should) still be used for both ui.apps and ui.content, but we may want one or two new checks to enforce the separation of apps and content.

davidjgonzalez commented 5 years ago

@adamcin any chance you could PR the separation checks to? https://github.com/davidjgonzalez/acs-aem-commons/tree/feature/1975

adamcin commented 5 years ago

One new check should suffice for requiring only apps/libs or only non-apps/libs nodes. Something like a CompositeStoreAlignmentCheck that generally wouldn't need additional configuration for AEM packages in general. We can configure Paths checks for each individual package to enforce which side of that distinction the package should be restricted to.

adamcin commented 5 years ago

@davidjgonzalez I'll submit a PR.

justinedelson commented 5 years ago

@davidjgonzalez see https://github.com/davidjgonzalez/acs-aem-commons/pull/5

davidjgonzalez commented 5 years ago

Just a FYI - I tried to update the cards to use /mnt/overlay/acs-commons/content/page-x.html however this was preventing certain objects from being exposed in the script (namely currentPage) which in turn broke some of the pages to be moved.

I changed these to use the pages' "real" paths /apps/acs-commons/content/page-x.html and everything works.

justinedelson commented 5 years ago

I was actually going to comment on that /mnt/overlay thing. Unless I've forgotten something significant, /apps is the end of the overlay chain, so unless we expect AEM to "underlay" this stuff (which would be weird), there's no reason to use /mnt/overlay.

davidjgonzalez commented 5 years ago

@adamcin i took a stab at adding the OakPAL checks here: https://github.com/davidjgonzalez/acs-aem-commons/commit/e0b3d7dc6eb828039122adbda8d777767a79efa9

I wanted to add an OakPAL check to the content container project, to ensure that its empty (other than the ACS sub packages) but it seemed to pick up the paths OF the sub packages and fail on them; not sure if there is simple way handle this.

Im very open to feedback on the rules I added - this is my first time OakPALing around!

adamcin commented 5 years ago

@davidjgonzalez it looks like a great start. I hadn't considered enforcing the behavior of the container package type, but that could definitely be useful as a reusable check in its own right. I'm working on that new check that I described, and based on my current implementation, it would fail the content package if it imported any of its own content outside of the /etc/packages/ root, AND if, in combination with its subpackages, it affected multiple composite mounts. If it doesn't affect any content outside of /etc/packages, I ignore any cross-mount effects of its subpackages in combination. If any individual subpackage affects multiple mounts, that subpackage will be reported as a violation.

davidjgonzalez commented 5 years ago

@adamcin sounds good! feel free to PR a complete replacement to my branch (i wont be offended ;))

davidjgonzalez commented 5 years ago

build_project_issues.csv.txt

Maybe as as followup issue - we should tackle atleast the Criticals in this report.

adamcin commented 5 years ago

Code Coverage is blocked because of this error:

INFO] ------------------------------------------------------------------------

[INFO] Skipping ACS AEM Commons - Reactor Project

[INFO] This project has been banned from the build due to previous failures.

[INFO] ------------------------------------------------------------------------

[INFO] ------------------------------------------------------------------------

[INFO] Reactor Summary:

[INFO] 

[INFO] ACS AEM Commons - Reactor Project .................. SUCCESS [ 14.668 s]

[INFO] ACS AEM Commons Bundle ............................. SUCCESS [03:02 min]

[INFO] ACS AEM Commons Oakpal Checks ...................... SUCCESS [ 18.670 s]

[INFO] ACS AEM Commons UI.Apps Package .................... SUCCESS [ 21.868 s]

[INFO] ACS AEM Commons UI.Content Package ................. SUCCESS [  2.812 s]

[INFO] ACS AEM Commons Package ............................ FAILURE [  0.542 s]

[INFO] ------------------------------------------------------------------------

[INFO] BUILD FAILURE

[INFO] ------------------------------------------------------------------------

[INFO] Total time: 04:13 min

[INFO] Finished at: 2019-08-11T14:31:15Z

[INFO] Final Memory: 98M/427M

[INFO] ------------------------------------------------------------------------

[ERROR] Failed to execute goal on project acs-aem-commons-content: Could not resolve dependencies for project com.adobe.acs:acs-aem-commons-content:content-package:4.3.1-SNAPSHOT: The following artifacts could not be resolved: com.adobe.acs:acs-aem-commons-ui.apps:zip:min:4.3.1-SNAPSHOT, com.adobe.acs:acs-aem-commons-ui.content:zip:min:4.3.1-SNAPSHOT: Could not find artifact com.adobe.acs:acs-aem-commons-ui.apps:zip:min:4.3.1-SNAPSHOT in adobe-public-releases (https://repo.adobe.com/nexus/content/groups/public/) -> [Help 1]

[ERROR] 

[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.

[ERROR] Re-run Maven using the -X switch to enable full debug logging.

[ERROR] 

[ERROR] For more information about the errors and possible solutions, please read the following articles:

[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/DependencyResolutionException

[ERROR] 

[ERROR] After correcting the problems, you can resume the build with the command

[ERROR]   mvn <goals> -rf :acs-aem-commons-content

Done. Your build exited with 0.
justinedelson commented 4 years ago

I believe this is complete.

ataimussova commented 4 years ago

There is an issue due to the split as rep:policy for apps are not being applied. The reason is that rep:policy is in ui.apps package and users are in ui.content. Due to the order of ui.apps being installed prior to ui.content, as a result system users have no access to /apps. It happens if you have no previous version of ACS AEM Commons installed because there are no ACS AEM Commons system users. Could you please fix?

joerghoh commented 4 years ago

@ataimussova can you raise a new issue for this? I guess that it might get ignored otherwise.

ataimussova commented 4 years ago

@joerghoh, thank you. The issue was already raised and fixed #2048.