CommentFileSizeAuthor
#9 interdiff-3176614-9.txt1.45 KBaadil.addweb
#9 3176614-9.patch23.73 KBaadil.addweb
#6 interdiff-3176614-6.txt6.26 KBjigish.addweb
#6 3176614-6.patch23.8 KBjigish.addweb
#2 3176614-2.patch20.66 KBaadil.addweb

Comments

binoli.addweb created an issue. See original summary.

aadil.addweb’s picture

aadil.addweb commented
Issue summary: View changes
StatusFileSize
new 3176614-2.patch20.66 KB
aadil.addweb’s picture

aadil.addweb commented
Status: Active » Needs review

@yas

Please review the patch for K8sCloudServerTemplatePluginTest Testcase.

Thanks

Status: Needs review » Needs work

The last submitted patch, 2: 3176614-2.patch, failed testing. View results

yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented

@binoliaddweb

Thank you for adding the test case.

+++ b/modules/cloud_service_providers/k8s/k8s.module
@@ -736,32 +736,8 @@ function k8s_namespace_allowed_values_function(FieldStorageConfig $definition, C
+ $k8s_service = \Drupal::service('k8s');

@@ -883,7 +859,8 @@ function k8s_cloud_server_template_presave(EntityInterface $entity) {
+  $k8s_service = \Drupal::service('k8s');

+++ b/modules/cloud_service_providers/k8s/src/Plugin/Validation/Constraint/YamlArrayDataConstraintValidator.php
@@ -14,9 +14,10 @@ class YamlArrayDataConstraintValidator extends ConstraintValidator {
+    $k8s_service = \Drupal::service('k8s');

+++ b/modules/cloud_service_providers/k8s/src/Plugin/Validation/Constraint/YamlObjectSupportConstraintValidator.php
@@ -13,11 +13,12 @@ class YamlObjectSupportConstraintValidator extends YamlArrayDataConstraintValida
+    $k8s_service = \Drupal::service('k8s');

+++ b/modules/cloud_service_providers/k8s/src/Plugin/Validation/Constraint/YamlUrlConstraintValidator.php
@@ -70,9 +70,10 @@ class YamlUrlConstraintValidator extends ConstraintValidator {
+            $k8s_service = \Drupal::service('k8s');

+++ b/modules/cloud_service_providers/k8s/tests/src/Functional/cloud/server_template/CloudServerTemplateTest.php
@@ -282,7 +282,8 @@ class CloudServerTemplateTest extends K8sTestBase {
+ $k8s_service = \Drupal::service('k8s');

Please use dependency injection (DI).

jigish.addweb’s picture

jigish.addweb commented
StatusFileSize
new 3176614-6.patch23.8 KB
new interdiff-3176614-6.txt6.26 KB
jigish.addweb’s picture

jigish.addweb commented
Status: Needs work » Needs review

@yas

Thank you for reviewing the patch. I changed the code with dependency injection.

Please review the updated patch.

Thanks

baldwinlouie’s picture

baldwinlouie commented
Status: Needs review » Needs work

@jigish.addweb and @yas,

Patch looks good. I just have the following comment. 

+++ b/modules/cloud_service_providers/k8s/k8s.module
@@ -736,32 +736,8 @@ function k8s_namespace_allowed_values_function(FieldStorageConfig $definition, C
+  $k8s_service = \Drupal::service('k8s');
+  return $k8s_service->supportedCloudServerTemplates();

I think we can just combine these statements

\Drupal::service('k8s')->supportedCloudServerTemplates();
aadil.addweb’s picture

aadil.addweb commented
Status: Needs work » Needs review
StatusFileSize
new 3176614-9.patch23.73 KB
new interdiff-3176614-9.txt1.45 KB

@baldwinlouie, @yas

Thank you for reviewing the patch, I refactored the code as per your comment.

Please review the new patch.

Thanks

Status: Needs review » Needs work

The last submitted patch, 9: 3176614-9.patch, failed testing. View results

aadil.addweb’s picture

aadil.addweb commented
Status: Needs work » Needs review
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Needs review » Reviewed & tested by the community

@binoliaddweb

Thank you for the update. I'll merge the patch to 8.x-2.x and 3.x and close this issue as Fixed.

  • yas committed 425d27e on 8.x-2.x authored by binoli.addweb 
    Issue #3176614 by binoli.addweb, jigish.addweb, yas, baldwinlouie: Add a...

  • yas committed f779342 on 3.x authored by binoli.addweb 
    Issue #3176614 by binoli.addweb, jigish.addweb, yas, baldwinlouie: Add a...
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Status: Reviewed & tested by the community » Fixed
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Version: 3.0.0-alpha2 » 3.x-dev
yas’s picture

yas
🇯🇵 he/him
Primary language Japanese
Location California 🇺🇸
commented
Related issues: +#3177284: Add a test case for K8sCloudProjectPluginTest

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.