OPENEUROPA-1739: Add services setup command. #104

dxvargas · 2019-08-23T06:04:48Z

OPENEUROPA-1739

Description

Add services setup command and tests.

voidtek

Somes remarks.

src/Commands/AbstractDrupalCommands.php

tests/Commands/DrupalCommandsTest.php

voidtek

Tested.

ademarco

The proposed solution does not implement what the issue is asking. What is proposed here can be achieved via YAML commands with a simple:

  setup:services:
    - { task: "process", source: "services.yml", destination: "${drupal.root}/sites/default/services.yml" }

We basically need to be able to have something like the following in the local runner.yml:

  service_parameters:
    cors.config:
      enabled: ${my_settings.cors.config_enabled}

And this to produce a sites/default/services.yml with:

parameters:
  service_parameters:
    cors.config:
      enabled: false

src/Commands/AbstractDrupalCommands.php

tests/fixtures/commands/drupal-services-setup.yml

ademarco

I tested on one of our components and it works well, I've just pointed out some changes, after this round I think we can close it.

ademarco · 2019-09-09T09:16:57Z

composer.json

@@ -10,12 +10,16 @@
        "consolidation/robo": "^1.4",
        "gitonomy/gitlib": "^1.0",
        "nuvoleweb/robo-config": "^0.2.1",
-        "jakeasmith/http_build_url": "^1.0.1"
+        "jakeasmith/http_build_url": "^1.0.1",
+        "symfony/yaml": ">3.1.0"


This should be checked against Symfony's major upgrading policy, we should come up with a proper version string here.

Not that Drupal core is already on 3.4.* which should be deprecation free.

Something like this

Ok 3.4 or 4.

ademarco · 2019-09-09T09:18:08Z

composer.json

    },
    "require-dev": {
        "openeuropa/code-review": "~1.0.0-beta3",
        "phpunit/phpunit": "~5.5||~6.0"
    },
+    "_readme": [
+        "We need require a version of 'symfony/yaml' higher then '3.1' since versions before 3.1.0 don't have dumper 'DUMP' constants available and 3.1.0 breaks permissions setup in tests."
+    ],


That's not needed, it's only needed when we fix dependencies when they break builds etc. not when we need a specific version because we use its API, for that having it in the requirements is enough.

For me there are two kind of dependencies. The first is when we need the package and it wouldn't be fetched if out of composer requirements. Second kind is like here when we're not really requiring the package since it was already being fetched, we're only adding it to adjust the accepted versions. For this later kind of dependencies I think it's good to understand why it was needed and to differentiate it since sometime soon we may remove it from the list, when other packages update their requirements.

Just saying, no problem, I've removed it.

ademarco · 2019-09-09T09:25:34Z

tests/Commands/DrupalCommandsTest.php

+            file_put_contents($services_destination_file, 'parameters: false');
+            if ($configs['force']) {
+                $force = ' --force';
+            }


Force is either true or not set at all, it's a valueless parameter, we can remove the inner if.

The "force" option in configuration wasn't applying directly to the force option in the command, it was also being used to know if a previous service.yml should happen.

But ok let's avoid confusion, I've changed the logic here and I think now it's clear.

ademarco · 2019-09-09T09:27:56Z

tests/fixtures/commands/drupal-services-setup.yml

+          settings:
+            foo: bar
+- config:
+    force: false


force: false is not a thing, 😄 it's either there or it's not. If you want to test it for false then don't set it.

ademarco · 2019-09-09T09:39:49Z

src/Commands/AbstractDrupalCommands.php

+        // Read given parameters.
+        $service_parameters['parameters'] = $this->getConfig()->get('drupal.service_parameters', []);
+        $dumper = new Dumper(2);
+        $yaml = $dumper->dump($service_parameters, PHP_INT_MAX, 0, Yaml::DUMP_EXCEPTION_ON_INVALID_TYPE);


Since this basically prints out a YAML file as Drupal expects to let's move this two lines in a separated protected method, so it can be used on all Drupal commands.

ademarco · 2019-09-09T09:40:39Z

src/Commands/AbstractDrupalCommands.php

+        $services_destination_file = $options['root'] . '/sites/' . $options['sites-subdir'] . '/services.yml';
+
+        $collection = [];
+        if (true === (bool) $options['force'] || !file_exists($services_destination_file)) {


$options['force'] is either true or false, Robo internals make sure of that, we don't need casting.

ademarco · 2019-09-09T09:55:53Z

tests/Commands/DrupalCommandsTest.php

+        $force = '';
+        if (isset($configs['force'])) {
+            mkdir($services_destination_dir, 0777, true);
+            file_put_contents($services_destination_file, 'parameters: false');


We should have 'parameters: false' as part of the fixture. If more comfortable you can create a separate test for the force option.

Ok, it's in fixture now.

…y requiring simfony/yaml >3.1.0

…meters in config.

…dency.

msnassar · 2019-12-16T14:05:40Z

src/Commands/AbstractDrupalCommands.php

+     *
+     * This command will add the configuration under service_parameters present in
+     * runner.yml into "services.yml" file in the site directory.
+     *


It would be nice to add example. Something like:

> drupal: > service_parameters: > session.storage.options: > cookie_lifetime: 0

msnassar · 2019-12-16T14:06:46Z

tests/fixtures/simulation.yml

+  configuration:
+    drupal:
+      service_parameters:
+        cors.config: false


This should be:

cors.config: enabled: false

msnassar · 2019-12-16T14:08:24Z

Tested... Works perfectly :)

msnassar · 2019-12-17T09:04:05Z

src/Commands/AbstractDrupalCommands.php

+        $service_parameters['parameters'] = $this->getConfig()->get('drupal.service_parameters', []);
+        $yaml = $this->dumpYaml($service_parameters);


This better go to the IF statement (line 349) as they are used in the statement only.

dxvargas force-pushed the OPENEUROPA-1739 branch 4 times, most recently from 5823b25 to 2186813 Compare August 27, 2019 11:37

voidtek requested changes Aug 27, 2019

View reviewed changes

voidtek previously approved these changes Aug 28, 2019

View reviewed changes

ademarco requested changes Aug 28, 2019

View reviewed changes

dxvargas dismissed voidtek’s stale review via e331535 August 29, 2019 14:12

ademarco force-pushed the OPENEUROPA-1739 branch from 1939266 to dea64da Compare September 3, 2019 07:57

ademarco requested changes Sep 3, 2019

View reviewed changes

src/Commands/AbstractDrupalCommands.php Outdated Show resolved Hide resolved

tests/fixtures/commands/drupal-services-setup.yml Show resolved Hide resolved

dxvargas force-pushed the OPENEUROPA-1739 branch 3 times, most recently from 365c822 to 88c15e2 Compare September 4, 2019 14:14

ademarco requested changes Sep 9, 2019

View reviewed changes

dxvargas force-pushed the OPENEUROPA-1739 branch from 22a76ff to f59b9c5 Compare September 9, 2019 17:16

D Vargas and others added 14 commits December 9, 2019 16:31

OPENEUROPA-1739: Add services setup command.

81da3e3

OPENEUROPA-1739: Add services setup tests.

0b01fdd

OPENEUROPA-1739: Add simulating test.

d1c58da

OPENEUROPA-1739: Fix code after review.

c2a63f9

OPENEUROPA-1739: Adapt to allow parameters to be set in runner file.

e93ba77

OPENEUROPA-1739: Adapt README file.

648fe0c

OPENEUROPA-1739: Add replacable parameters in test.

19e698e

OPENEUROPA-1739: Add more tests.

88bc406

OPENEUROPA-1739: Improve dumped YAML format and tests.

55eda92

OPENEUROPA-1739: Add tests for force option.

07ddf79

OPENEUROPA-1739: Ensure compatibility with version 2 of simfony/yaml.

9cea0e2

OPENEUROPA-1739: Ensure Yaml::DUMP_EXCEPTION_ON_INVALID_TYPE exists b…

beeb546

…y requiring simfony/yaml >3.1.0

OPENEUROPA-1739: Ensure services file is created without service_para…

0d6d4db

…meters in config.

OPENEUROPA-1739: Fix code after review.

fc5a85d

ademarco force-pushed the OPENEUROPA-1739 branch from f59b9c5 to fc5a85d Compare December 9, 2019 15:33

OPENEUROPA-1739: OPENEUROPA-1739: Add cweagans/composer-patches depen…

a5a6440

…dency.

dxvargas force-pushed the OPENEUROPA-1739 branch from 3e845d9 to a5a6440 Compare December 12, 2019 12:02

msnassar reviewed Dec 16, 2019

View reviewed changes

tests/fixtures/simulation.yml

configuration:

drupal:

service_parameters:

cors.config: false

Copy link

msnassar Dec 16, 2019

Choose a reason for hiding this comment

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

This should be:

cors.config: enabled: false

msnassar reviewed Dec 17, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OPENEUROPA-1739: Add services setup command. #104

OPENEUROPA-1739: Add services setup command. #104

dxvargas commented Aug 23, 2019

voidtek left a comment

voidtek left a comment

ademarco left a comment

ademarco left a comment

ademarco Sep 9, 2019

dxvargas Sep 9, 2019 •

edited

Loading

ademarco Sep 9, 2019

dxvargas Sep 9, 2019 •

edited

Loading

ademarco Sep 9, 2019

dxvargas Sep 9, 2019 •

edited

Loading

ademarco Sep 9, 2019

dxvargas Sep 9, 2019 •

edited

Loading

ademarco Sep 9, 2019

ademarco Sep 9, 2019

dxvargas Sep 9, 2019

ademarco Sep 9, 2019

dxvargas Sep 9, 2019

msnassar Dec 16, 2019

msnassar Dec 16, 2019

msnassar commented Dec 16, 2019

msnassar Dec 17, 2019

		$service_parameters['parameters'] = $this->getConfig()->get('drupal.service_parameters', []);
		$yaml = $this->dumpYaml($service_parameters);

OPENEUROPA-1739: Add services setup command. #104

Are you sure you want to change the base?

OPENEUROPA-1739: Add services setup command. #104

Conversation

dxvargas commented Aug 23, 2019

OPENEUROPA-1739

Description

voidtek left a comment

Choose a reason for hiding this comment

voidtek left a comment

Choose a reason for hiding this comment

ademarco left a comment

Choose a reason for hiding this comment

ademarco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxvargas Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxvargas Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxvargas Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dxvargas Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msnassar commented Dec 16, 2019

Choose a reason for hiding this comment

dxvargas Sep 9, 2019 •

edited

Loading

dxvargas Sep 9, 2019 •

edited

Loading

dxvargas Sep 9, 2019 •

edited

Loading

dxvargas Sep 9, 2019 •

edited

Loading