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

#2589 - Fixes issues with 6.4.8 compat #2590

Merged

Conversation

davidjgonzalez
Copy link
Contributor

  • Issues with too-modern repo init set properties
  • Issues with sling service user mapping

@HitmanInWis This works on 6.4.8 -- need to double-check on 6.5.x and AEM CS

- Issues with too-modern repo init set properties
- Issues with sling service user mapping
@coveralls
Copy link

coveralls commented May 21, 2021

Pull Request Test Coverage Report for Build 6441

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 57.366%

Totals Coverage Status
Change from base Build 6439: 0.0%
Covered Lines: 17246
Relevant Lines: 30063

💛 - Coveralls

<jcr:root xmlns:sling="http://sling.apache.org/jcr/sling/1.0" xmlns:jcr="http://www.jcp.org/jcr/1.0"
jcr:primaryType="sling:OsgiConfig"
user.default=""
user.mapping="[com.adobe.acs.acs-aem-commons-bundle:ensure-oak-index=[acs-commons-ensure-oak-index-service],com.adobe.acs.acs-aem-commons-bundle:email-service=[acs-commons-email-service],com.adobe.acs.acs-aem-commons-bundle:httpcache-jcr-storage-service=[acs-commons-httpcache-jcr-storage-service],com.adobe.acs.acs-aem-commons-bundle:review-task-asset-mover=[acs-commons-review-task-asset-mover-service],com.adobe.acs.acs-aem-commons-bundle:error-page-handler=[acs-commons-error-page-handler-service],com.adobe.acs.acs-aem-commons-bundle:form-helper=[acs-commons-form-helper-service],com.adobe.acs.acs-aem-commons-bundle:dispatcher-flush=[acs-commons-dispatcher-flush-service],com.adobe.acs.acs-aem-commons-bundle:package-replication-status-event-listener=[acs-commons-package-replication-status-event-service],com.adobe.acs.acs-aem-commons-bundle:component-error-handler=[acs-commons-component-error-handler-service],com.adobe.acs.acs-aem-commons-bundle:system-notifications=[acs-commons-system-notifications-service],com.adobe.acs.acs-aem-commons-bundle-twitter:twitter-updater=[acs-commons-twitter-updater-service],com.adobe.acs.acs-aem-commons-bundle:workflow-remover=[acs-commons-workflow-remover-service],com.adobe.acs.acs-aem-commons-bundle:bulk-workflow=[acs-commons-bulk-workflow-servic],com.adobe.acs.acs-aem-commons-bundle:bulk-workflow-runner=[workflow-process-service],com.adobe.acs.acs-aem-commons-bundle:ensure-service-user=[acs-commons-ensure-service-user-service],com.adobe.acs.acs-aem-commons-bundle:shared-component-props=[acs-commons-shared-component-props-service],com.adobe.acs.acs-aem-commons-bundle:manage-controlled-processes=[acs-commons-manage-controlled-processes-service],com.adobe.acs.acs-aem-commons-bundle:automatic-package-replicator=[acs-commons-automatic-package-replicator-service],com.adobe.acs.acs-aem-commons-bundle:on-deploy-scripts=[acs-commons-on-deploy-scripts-service],com.adobe.acs.acs-aem-commons-bundle:remote-assets=[acs-commons-remote-assets-service],com.adobe.acs.acs-aem-commons-bundle:workflowpackagemanager-service=[acs-commons-workflowpackagemanager-service],com.adobe.acs.acs-aem-commons-bundle:redirect-manager=[acs-commons-manage-redirects-service],com.adobe.acs.acs-aem-commons-bundle:file-fetch=[acs-commons-file-fetch-service]]"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for this to be stored on multiple lines to ease up on merge conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh - good idea.

@@ -20,7 +26,7 @@ set ACL for anonymous
allow jcr:read on /conf restriction(rep:glob,/*/settings/redirects)
end

create service user acs-commons-automatic-package-replicator-service with path system/acs-commons
create service user acs-commons-automatic-package-replicator-service with path system/acs-aem-commons
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this path change?


# Not supported in 6.4.8
#set properties on /conf/global/settings/redirects
# set sling:resourceType{String} to acs-commons/components/utilitiacs-commons-automatic-package-replicator-servicees/manage-redirects/redirects
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's commented out, but this line got accidentally mangled in the last commit

com.adobe.acs.acs-aem-commons-bundle:file-fetch=[acs-commons-file-fetch-service]
]"/>

<!-- sling:OsgiConfig format used due to 6.4.8 support -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even AEM 6.4 supports the better suited .config format (https://sling.apache.org/documentation/bundles/configuration-installer-factory.html#configuration-files-config). I would not recommend adding new configuration with sling:OsgiConfig format which has a lot of flaws: https://sling.apache.org/documentation/bundles/configuration-installer-factory.html#slingosgiconfig-resources

Copy link
Contributor Author

@davidjgonzalez davidjgonzalez May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these any caveats with .config? That's what we had before and when installed no service user mappings would get registered. Moving them back to sling:OsgiConfig and they work just fine. The .config did work on 6.5 and AEM CS though... thoughts on why 6.4 was problematic?

Does master (which uses .config) work for you on 6.4.8?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I know of. In case there is issues with .config on 6.4.8 this is probably due to escaping. Any logs available from OSGi installer with the problematic .config?

Copy link
Contributor

@kwin kwin May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the problem is not the .config, but the filename. Tilde as separator is only supported since Configuration Factory Packages 1.2.0 (https://issues.apache.org/jira/browse/SLING-7786) while AEM 6.4 still ships with 1.1.x. So just converting the filename from ui.apps/src/main/content/jcr_root/apps/acs-commons/config/org.apache.sling.serviceusermapping.impl.ServiceUserMapperImpl.amended~acs-commons.config to ui.apps/src/main/content/jcr_root/apps/acs-commons/config/org.apache.sling.serviceusermapping.impl.ServiceUserMapperImpl.amended-acs-commons.config should be enough to fix this.

@davidjgonzalez When you converted to sling:OsgiConfig format you also changed the filename separator. Thats why it was working with this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sunuva. didnt even see that .. 👓 ...
Ill flip it back and update to -..

@HitmanInWis
Copy link
Contributor

FYI if this is waiting on my review/merge I will get to it, but if someone is able to get to it before me please feel free :)

@kwin
Copy link
Contributor

kwin commented May 25, 2021

@davidjgonzalez Can you add a changelog entry as well?

@davidjgonzalez davidjgonzalez merged commit e85ef74 into Adobe-Consulting-Services:master May 25, 2021
@davidjgonzalez davidjgonzalez deleted the fix/2589 branch May 25, 2021 20:19
@davidjgonzalez davidjgonzalez modified the milestone: 4.11.4 May 25, 2021
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.

4 participants