ACS Commons fails to deploy to AEM as a Cloud Service due to inclusion of /var nodes

davidjgonzalez commented 4 years ago

Required Information

Originally reported with clear description of symptoms by @stefanseifert at: https://github.com/Adobe-Consulting-Services/acs-aem-commons/issues/2334

[X] AEM Version, including Service Packs, Cumulative Fix Packs, etc: AEM as a Cloud Service
[X] ACS AEM Commons Version: 4.x.x
[X] Reproducible on Latest? yes

ACS AEM Commons fails to deploy to AEM as a Cloud Service because AEM's replication service user does not support replication (due to permissions of the service user) or nodes under /var, which the ACS Commons ui.content project has.

These nodes should should be establishing using repoinit scripts (targetting the minimal runmodes) as needed. This looks trivial for all nodes EXCEPT /var/acs-commons/reports which contains cq:Page definitions (which are large, and complex in structure).

@klcodanr is there a reason these nodes are in /var? It actually seems strange these are in /var which historically has more transient, runtime content (these appear to be report definitions). Thoughts on if they could be easily moved outside of /var?

Also, can you help me confirm:

[x] /var/acs-commons/httpcache -> Should exist on Author and Publish (@niekraaijmakers)
[ ] /var/acs-commons/mcp -> Should exist only on Author (@badvision)
[x] /var/acs-commons/on-deploy-scripts -> Should exist on Author and Publish (@HitmanInWis)
[ ] /var/acs-commons/reports -> Should exist only on Author (@klcodanr)

The working PR to resolve this is at: https://github.com/Adobe-Consulting-Services/acs-aem-commons/pull/2342

klcodanr commented 4 years ago

@davidjgonzalez

We should be able to relocate. As a bridge, the listing component could be changed to look in two locations whereas new reports would be created in the new location and either add a feature or a note for AEM Cloud Service users telling them to relocate reports. There are a couple options I could think of:

1. Put under /content

We could move to /content/acs-commons/reports or similar path. There are a couple of downsides:

It makes me uncomfortable to put non-publishable content under the /content tree, especially since it's a reporting feature, which would seem bad to accidentally publish
You can't exclude a folder from the Sites view without overlaying /libs because the excluded pattern is set in properties at: /libs/wcm/core/content/sites/jcr:content/views/column/datasource@exclude

2. Put under /conf

We could move to /conf/acs-commons/reports or similar path. I'm not 100% sure on whether this would work since it seems like /var should have been fine as well based on the documentation: https://docs.adobe.com/content/help/en/experience-manager-cloud-service/implementing/developing/aem-project-content-package-structure.html

If it does work, that would seem like a better approach as /conf is not going to generally be enabled for web access.

Definitely open to other options.

HitmanInWis commented 4 years ago

Yes /var/acs-commons/on-deploy-scripts should exist on author/publish - many times On-Deploy Scripts do content refactors that we dont want to require a publish event if it can be avoided.

davidjgonzalez commented 4 years ago

Thanks @klcodanr - it seems like if we have an idea of where we'd like to "eventually" move all the utility pages from /etc/acs-commons, it would be good to make align to that.

Do the reports cq:Page sub-trees need to be authorable (persistable?) and/or would a customer add/modifiable? If not - could we move them to /apps perhaps? (the filter.xml rule makes it seem like customers might be adding/modifying this area tho)

I too share your concerns of creating /content/acs-commons .. it just doesn't feel right to me, as the SiteAdmin (which surfaces /content) is well, the "Site admin" ... it should list the web sites/properties hosted by AEM.

I want to say @kwin had thoughts on the larger /etc/acs-commons move .. but dont have the GH issue handy.

klcodanr commented 4 years ago

Argh, I misread your initial comment, so yes, the reports need to be authorable, it wouldn't make a lot of sense to move them to /apps. IMO - /conf seems like the best option.

badvision commented 4 years ago

configuring a report is analogous to configuring a template or content fragment model -- I support the idea of /conf. I don't think there's an API that standardizes how we use that area so proceed with caution. Might be worth abstracting a model for working with conf, I feel like we're going to be here again.

badvision commented 4 years ago

Keep in mind that the dialog generator is now enabled by default, so it could be as simple as a sling model that explains the configuration stored in /conf and a little wiring to access the dialog to edit those values.

davidjgonzalez commented 4 years ago

@badvision I think the larger problem w/ /conf is there isn't a simple, OOTB framework for managing the pages themselves (rather than content IN the pages). This was the big benefit of /miscadmin .. it basically cloned the siteadmin capabilities outside the context of Sites... unless i missed something and the Dialog Generator can also generate that page administrator experience.

badvision commented 4 years ago

It cannot -- and you hit the nail on the head. That was the challenge I ran into switching generic lists to use dialog generator; I had to clone the site admin console and hack at the code until things fit right. I would like to not repeat that process as it doesn't scale and is a bear to maintain as well I'm sure.

badvision commented 4 years ago

But we need something like /miscadmin, a generic console that adapts to the types we need to manage through it, what toolbars are available, etc. These were never abstracted well, and it doesn't seem like the product is on track to do that for us. However, dialog generation scales. Console generation should also scale as well. It's just a different type of abstraction that results in synthetic resources.

badvision commented 4 years ago

Like dialog generation, if the console generation is no longer compatible at some future revision, then a good abstraction of what a console should provide will still hold up, we just change the generator to match what the product needs and use version detection or something similar so it uses the right implementation. Will need to give it some thought.

klcodanr commented 4 years ago

Agreed 100% @davidjgonzalez and @badvision -- I would also mention that if we were to do this, I would recommend a "clean room" approach to the implementation vs relying on CoralUI. Coral UI has the "fun" tendency to have incompatible changes between versions that break things in "exciting" ways.

https://blogs.perficient.com/2020/04/13/building-aem-admin-consoles-that-will-not-break-with-new-aem-releases/ (sorry for the broken images, I'll bug our website people)

badvision commented 4 years ago

@klcodanr Agreed 100% -- it needs to be abstracted, e.g. a UML-like breakdown of what is needed at the core. The UI is an implementation of that abstraction and should not inform the core model. If we do that, it should be possible to keep it relevant regardless of UI changes. I've never been a fan of the death-by-convention approach used pervasively, but fortunately the open design of the product allows plenty of room for clean abstractions to co-exist with legacy approaches. So it's not all bad, we have room to grow here.

royteeuwen commented 4 years ago

I don't understand why editing of pages in /conf is not OOTB included? We just copy pasted /sites.html and made it /conf.html so that you get the full page UI editor. It all works, we had to do almost no adjustments at all. This way you can just make new conf pages like you would in /miscadmin

On 24 Jun 2020, at 17:43, Brendan Robert notifications@github.com wrote:

@klcodanr https://github.com/klcodanr Agreed 100% -- it needs to be abstracted, e.g. a UML-like breakdown of what is needed at the core. The UI is an implementation of that abstraction and should not inform the core model. If we do that, it should be possible to keep it relevant regardless of UI changes. I've never been a fan of the death-by-convention approach used pervasively, but fortunately the open design of the product allows plenty of room for clean abstractions to co-exist with legacy approaches. So it's not all bad, we have room to grow here.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Adobe-Consulting-Services/acs-aem-commons/issues/2341#issuecomment-648900522, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABF42TQEMTAL65J77B52XYDRYINJHANCNFSM4OGWOJ6A.

badvision commented 4 years ago

like

I thought the same thing when I wanted to add touchui admin interface for generic lists but it is not as genetically defined as I thought it was and needed to be cloned to remove extra junk not needed such as versions, etc.

niekraaijmakers commented 4 years ago

Httpcache: both author and publish

stefanseifert commented 4 years ago

this problem is not yet fixed in the latest 4.7.2 release. is there a time plan for this issue? - imho this completely block projects deploying to AEM cloud using ACS AEM Commons.

davidjgonzalez commented 4 years ago

This needs /var/acs-commons/reporting to be moved out (i guess to /etc/acs-commons/reporting .. i wish we had a better vision for where these should land so we dont have to move them again)

This would also be a breaking change, for folks that have created reports under /var, so it'd have to roll into v5.0.0.

@klcodanr any luck finding time to move the reports?

HitmanInWis commented 4 years ago

If we're discussing a v5.0.0, would this be a good time to drop support for 6.3 and possibly even 6.4 for v5.0.0?

davidjgonzalez commented 4 years ago

@HitmanInWis the plan was to drop 6.3 support in v5.0.0.

The 5.0.0 release is tracked here: https://github.com/Adobe-Consulting-Services/acs-aem-commons/issues/2288

If you have a compelling pitch for dropping 6.4, drop in there! :)

badvision commented 4 years ago

For starters, 6.5's been out for a year...

HitmanInWis commented 4 years ago

I know what @badvision says is a bit tongue in cheek, but truthfully we should be on 6.6 now (instead we have AEMaaCS) so really 6.4 is currently 2 versions old. Furthermore, new features contributed are expected to be 6.5+CS capable. I'm just not sure the 6.4 support is really compelling. When does 6.4 go EOL for Adobe support?

davidjgonzalez commented 4 years ago

Is 6.4 compat preventing us from doing anything we want to do? Can we list the benefits of dropping 6.4 support? I understand it's old, but people are, for better or worse, still on 6.4.x and i believe Adobe still release SP's for 6.4 (or did in May, https://docs.adobe.com/content/help/en/experience-manager-64/release-notes/sp-release-notes.html)

If we can support 6.4 at no cost to us, then why not? If there is a cost, what is it?

klcodanr commented 4 years ago

Just committed an approach to moving the reports under /conf in #2365 -- IMO we could also look into removing the report console entirely, though we'd have to consider how to handle existing reports under /var

stefanseifert commented 4 years ago

the problem still exists in ACS AEM Commons 4.8.4. (is no one using ACS AEM Commons in AEM as a cloud service? it's not even possible to deploy it there.)

davidjgonzalez commented 4 years ago

@stefanseifert yeh - have to release 5.0.0 to incorporate @klcodanr 's fix ... havent found the time. maybe this weekend?

Just a mental reminder, i think we have to adjust services users to be defined in [ ] as well so they can be used w/ prinicipal based ACLs (i should make an issue for this)

https://github.com/Adobe-Consulting-Services/acs-aem-commons/issues/2288

stefanseifert commented 3 years ago

gentle nudge - problem is still not solved, unable to deploy ACS AEM commons on AEM as a cloud service

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kwin commented 3 years ago

@davidjgonzalez @klcodanr What do you think about a minimal fix for the 4.x versions to create the /var content structure via repoinit? That requires fewer changes and should not cause regressions. I fear that 5.x will no happen too soon and this blocks usage of ACS AEM Commons with AEMaaCS (despite what is written in https://adobe-consulting-services.github.io/acs-aem-commons/pages/compatibility.html#aem-as-a-cloud-service-feature-incompatibility)

davidjgonzalez commented 3 years ago

@kwin we could - that would be a breaking change - so 5.x right?

I have the 5.x branch which I think should be the last of it - I think the biggest thing is doing a regression since so many filters changed.

If you think we can do a simple adjustment to 5.x and bump what's prepped for 5.x to 6.x

Agree 5.x is dragging way too long (I just had the time outside work to work on it :()

kwin commented 3 years ago

that would be a breaking change

IMHO not, my proposal is to leave everything at /var but just create the content there with repoinit instead of content package. That is no breaking change and should work with AEM Classic and Cloud.

Not migrating would also mean though, that you cannot replicate any nodes below /vars in general from author to publish, which would prevent the usage of some of the features (HTTPCache and OnDeployScripts) but at least ACS AEM Commons in general would be usable!

kwin commented 3 years ago

@davidjgonzalez Is this considered a valid workaround: https://helpx.adobe.com/in/experience-manager/kb/cm/cloudmanager-deploy-fails-due-to-sling-distribution-aem.html?

If so we could add write permissions to the acs-aem-commons specific var folders for the sling-distribution-importer to a repoinit section. If you are fine with that approach, I will prepare a PR for 4.x.

Talking out loud - Wrt to back compat (I always forget what versions of AEM support repoinit) - it should be fine, since we only need these ACLs ON AEM CS, which 💯 does support repo init. Even if an older (ACS Commons) Supported version doesnt support repoinit, its ok since 6.x isnt impacted by this need for permissions.

That said, helpx appears to be down right now? But i assume the KB provides public instruction to add these ACLs via repoinit.

davidjgonzalez commented 3 years ago

@kwin agh - sorry. I need to click into Github and not do stuff via GH emails :)

Applying the ACLs directly to/var/acs-commons seems like a good interim solution. I dont think it's reasonable for anyone else to piggy-back on this space, since the folder (/var/acs-commons) is well named, and when we move things out of /var entirely, that's breaking change that encompasses a change to these permissions.

kwin commented 3 years ago

Just for the record: The default permissions of the system user on the publish are listed in https://github.com/Netcentric/aem-cloud-validator/issues/9#issuecomment-802121462.

Adobe-Consulting-Services / acs-aem-commons