Re: [VOTE] Release Apache Sling JCR Resource 3.3.2
On Mon, 2024-05-20 at 16:02 +, Radu Cotescu wrote: > Please vote to approve this release: +1 Robert signature.asc Description: This is a digitally signed message part
Re: [PR] SLING-12283 : write config in correct format incase of factoryPid is … [sling-org-apache-sling-installer-provider-jcr]
rishabhdaim commented on code in PR #9: URL: https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9#discussion_r1608439188 ## src/main/java/org/apache/sling/installer/provider/jcr/impl/JcrInstaller.java: ## @@ -707,6 +716,7 @@ private UpdateResult handleUpdate(final String resourceType, dataNode.setProperty(PROP_MODIFIED, Calendar.getInstance()); dataNode.setProperty(PROP_ENC, ENCODING); dataNode.setProperty(PROP_MIME, MIME_TXT); +dataNode.setProperty(ORIGINAL_PID, id); Review Comment: I didn't get any exceptions while creating a config on AEM 1, and it was successfully replicated on AEM 2. I set up both instances using Mongo document store. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12283 : changed the config Pid to separate factoryPID & PID wit… [sling-org-apache-sling-installer-factory-configuration]
rishabhdaim commented on code in PR #13:
URL:
https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#discussion_r1608464507
##
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigInstallTask.java:
##
@@ -92,6 +92,9 @@ public void execute(final InstallationContext ctx) {
}
Configuration config =
ConfigUtil.getConfiguration(this.getConfigurationAdmin(), this.factoryPid,
this.configPid);
+if (config == null) {
+config =
ConfigUtil.getLegacyFactoryConfig(this.getConfigurationAdmin(),
this.factoryPid, null, this.configPid);
+}
Review Comment:
I agree that I haven't checked whether the `original.pid` property is
present or not cause I didn't need it (may be we can remove it altogether).
Regarding the checking of config, the `ConfigUtil.getLegacyFactoryConfig`
does check it
[here](https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/blob/master/src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java#L251-L256)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12283 : write config in correct format incase of factoryPid is … [sling-org-apache-sling-installer-provider-jcr]
rishabhdaim commented on code in PR #9:
URL:
https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9#discussion_r1608455987
##
src/main/java/org/apache/sling/installer/provider/jcr/impl/JcrUtil.java:
##
@@ -73,4 +75,21 @@ public static Node createPath(final Session session,
}
return parentNode.getNode(relativePath);
}
+
+/**
+ * Get the PID for a configuration event by using the R7 format before
saving it.
+ *
+ * @param factoryPid factory PID of the configuration
+ * @param pid PID of the configuration
+ * @return The PID in R7 format
+ */
+public static String getPid(final String factoryPid, final String pid) {
+// if factory pid is separated from pid by a period (.), we need
replace it with a ~ so that this can be installed
+// and grouped as a factory configuration
+if (pid.startsWith(factoryPid + '.')) {
+String id = pid.substring(factoryPid.length() + 1);
+return factoryPid + "~" + id;
+}
+return pid;
Review Comment:
Addressed in
https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9/commits/6b9cdd225224636759cd64a96328c9b573316aff
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12283 : write config in correct format incase of factoryPid is … [sling-org-apache-sling-installer-provider-jcr]
rishabhdaim commented on code in PR #9: URL: https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9#discussion_r1608439188 ## src/main/java/org/apache/sling/installer/provider/jcr/impl/JcrInstaller.java: ## @@ -707,6 +716,7 @@ private UpdateResult handleUpdate(final String resourceType, dataNode.setProperty(PROP_MODIFIED, Calendar.getInstance()); dataNode.setProperty(PROP_ENC, ENCODING); dataNode.setProperty(PROP_MIME, MIME_TXT); +dataNode.setProperty(ORIGINAL_PID, id); Review Comment: I didn't get any exceptions while creating a config on AEM 1 and it gets replicated on AEM 2 successfully. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] SLING-12283 : write config in correct format incase of factoryPid is … [sling-org-apache-sling-installer-provider-jcr]
jsedding commented on code in PR #9:
URL:
https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9#discussion_r1608329623
##
src/main/java/org/apache/sling/installer/provider/jcr/impl/JcrUtil.java:
##
@@ -73,4 +75,21 @@ public static Node createPath(final Session session,
}
return parentNode.getNode(relativePath);
}
+
+/**
+ * Get the PID for a configuration event by using the R7 format before
saving it.
+ *
+ * @param factoryPid factory PID of the configuration
+ * @param pid PID of the configuration
+ * @return The PID in R7 format
+ */
+public static String getPid(final String factoryPid, final String pid) {
+// if factory pid is separated from pid by a period (.), we need
replace it with a ~ so that this can be installed
+// and grouped as a factory configuration
+if (pid.startsWith(factoryPid + '.')) {
+String id = pid.substring(factoryPid.length() + 1);
+return factoryPid + "~" + id;
+}
+return pid;
Review Comment:
I would flip the logic. I.e. let a pid in the correct format through. If the
format is incorrect, normalize the pid to a name by removing a prefix of
`factoryPid` if present. Then concat the factoryPid, ~ and normalized pid.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12283 : changed the config Pid to separate factoryPID & PID wit… [sling-org-apache-sling-installer-factory-configuration]
jsedding commented on code in PR #13:
URL:
https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#discussion_r1608323556
##
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigInstallTask.java:
##
@@ -92,6 +92,9 @@ public void execute(final InstallationContext ctx) {
}
Configuration config =
ConfigUtil.getConfiguration(this.getConfigurationAdmin(), this.factoryPid,
this.configPid);
+if (config == null) {
+config =
ConfigUtil.getLegacyFactoryConfig(this.getConfigurationAdmin(),
this.factoryPid, null, this.configPid);
+}
Review Comment:
I don't think this is a complete fix (together with
https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9).
The `original.pid` property is not leveraged. Installation would need to be
skipped if the `original.pid` property is present AND a configuration with that
PID exists in config admin.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] SLING-12283 : write config in correct format incase of factoryPid is … [sling-org-apache-sling-installer-provider-jcr]
jsedding commented on code in PR #9: URL: https://github.com/apache/sling-org-apache-sling-installer-provider-jcr/pull/9#discussion_r1608319420 ## src/main/java/org/apache/sling/installer/provider/jcr/impl/JcrInstaller.java: ## @@ -707,6 +716,7 @@ private UpdateResult handleUpdate(final String resourceType, dataNode.setProperty(PROP_MODIFIED, Calendar.getInstance()); dataNode.setProperty(PROP_ENC, ENCODING); dataNode.setProperty(PROP_MIME, MIME_TXT); +dataNode.setProperty(ORIGINAL_PID, id); Review Comment: Does this work, seeting a property on the `jcr:content` node? I think the `nt:resource` node type is quite restrictive. IIRC Oak uses a different node-type for a file's `jcr:content` node. Maybe that one is more lenient. Alternatively, a mixin nodetype could be added to allow for the extra property. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Update dependency org.codehaus.mojo:build-helper-maven-plugin to v3.6.0 [sling-site]
rombert merged PR #164: URL: https://github.com/apache/sling-site/pull/164 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
