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_r1611600431
##
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 think this change here is actually "good enough".
--
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 : 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 : changed the config Pid to separate factoryPID & PID wit… [sling-org-apache-sling-installer-factory-configuration]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2120007654 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [0 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=WONTFIX) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2113317468 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2113316059 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2113311666 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2113206440 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [7 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
rombert commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2112452359 @rishabhdaim - can you please merge with the latest master commit? https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/commit/fd459daca46c60aeac53c33e699a8973111ba4cf would make our testing easier. -- 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]
rombert commented on code in PR #13:
URL:
https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#discussion_r1601372208
##
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java:
##
@@ -336,4 +337,20 @@ public static void removeRedundantProperties(final
Dictionary pr
}
}
}
+
+public static String getPid(final ConfigurationEvent event) {
+final String id;
+final String pid;
+if (event.getFactoryPid() == null ) {
Review Comment:
I think the code would be clearer if you would check if you need to after
the pid ( branch on line 347 ) and then have a fallback to `id =
event.getPid();`.
##
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigTaskCreator.java:
##
@@ -131,10 +131,11 @@ public InstallTask createTask(final TaskResourceGroup
group) {
@Override
public void configurationEvent(final ConfigurationEvent event) {
synchronized ( Coordinator.SHARED ) {
+final String id = ConfigUtil.getPid(event);
Review Comment:
I think we need a more significant name than "id".
##
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigTaskCreator.java:
##
@@ -131,10 +131,11 @@ public InstallTask createTask(final TaskResourceGroup
group) {
@Override
public void configurationEvent(final ConfigurationEvent event) {
synchronized ( Coordinator.SHARED ) {
+final String id = ConfigUtil.getPid(event);
if ( event.getType() == ConfigurationEvent.CM_DELETED ) {
final Coordinator.Operation op =
Coordinator.SHARED.get(event.getPid(), event.getFactoryPid(), true);
Review Comment:
It would be good to document somewhere by we pass the original PID and
Factory PID to the coordinator but the recalculated one elsewhere.
##
src/main/java/org/apache/sling/installer/factories/configuration/impl/ConfigUtil.java:
##
@@ -336,4 +337,20 @@ public static void removeRedundantProperties(final
Dictionary pr
}
}
}
+
Review Comment:
Please document why this is needed
--
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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2096967292 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [2 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2049658267 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate passed** Issues  [2 New issues](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [0 Accepted issues](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_accepted_issues=list) Measures  [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-installer-factory-configuration=13=false=true)  [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list)  [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_duplicated_lines_density=list) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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]
sonarcloud[bot] commented on PR #13: URL: https://github.com/apache/sling-org-apache-sling-installer-factory-configuration/pull/13#issuecomment-2046510173 ## [](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) **Quality Gate failed** Failed conditions  [68.8% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-installer-factory-configuration=13=new_coverage=list) (required ≥ 80%) [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-installer-factory-configuration=13) -- 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
