Add YAML configuration files for micro services #8

jonathangreen · 2017-04-17T17:00:40Z

GitHub Issue

Part of:
Islandora/documentation#603

What does this Pull Request do?

This PR does a few things:

It puts the service providers in their own provider subdirectory
It updates the IslandoraServiceProvider to be more idiomatic in how it handles configuration loading etc.
It adds a new service provider YamlConfigServiceProvider that loads configuration from YAML files and registers them as properties on the container.

How should this be tested?

Should be tested along with the forthcoming PR to Crayfish to use these new services.

Interested parties

@Islandora-CLAW/committers

ruebot · 2017-04-17T17:04:33Z

src/Provider/IslandoraServiceProvider.php

@@ -0,0 +1,91 @@
+<?php


@jonathangreen did you do a mv or git mv here?

If you look here:
fc681e9

It is a rename.

ah, cool. I guess it's showing up differently in the diff, or I should just look at the commits themselves 😃

codecov · 2017-04-17T17:16:02Z

Codecov Report

Merging #8 into master will increase coverage by 13.55%.
The diff coverage is 94.11%.

@@              Coverage Diff              @@
##             master       #8       +/-   ##
=============================================
+ Coverage     62.79%   76.34%   +13.55%     
- Complexity      104      114       +10     
=============================================
  Files             8        9        +1     
  Lines           344      372       +28     
=============================================
+ Hits            216      284       +68     
+ Misses          128       88       -40

Impacted Files	Coverage Δ	Complexity Δ
src/Syn/SettingsParser.php	`90.98% <100%> (+0.98%)`	`47 <0> (+2)`	⬆️
src/Provider/IslandoraServiceProvider.php	`92.85% <92.85%> (ø)`	`4 <4> (?)`
src/Provider/YamlConfigServiceProvider.php	`95.45% <95.45%> (ø)`	`10 <10> (?)`
src/Syn/JwtAuthenticator.php	`97.61% <0%> (+1.19%)`	`23% <0%> (ø)`	⬇️
src/CmdExecuteService.php	`4.25% <0%> (+4.25%)`	`12% <0%> (ø)`	⬇️
src/FedoraResourceConverter.php	`25% <0%> (+25%)`	`3% <0%> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdd33fb...a6b4b6e. Read the comment docs.

* Update IslandoraServiceProvider to be more idiomatic in the way it uses configuration variables. * Add a new YamlConfigServiceProvider to load configuration from YAML files for Crayfish services.

whikloj

Not sure I understand the YamlConfigServiceProvider. I don't know where the crayfish configuration parameters would come from, and I'm not sure if I like all the configuration stuff being placed into the container along side all the actual services/providers...

Questions/comments below

whikloj · 2017-04-18T03:00:39Z

src/Provider/IslandoraServiceProvider.php

+            return strtolower($container['crayfish.log.level']) == 'none' ? null : $container['crayfish.log.file'];
+        };
+        $container['monolog.level'] = function ($container) {
+            return $container['crayfish.log.level'];


We don't have a "crayfish" application that runs, so I think this should probably be $container['gemini.log.level'] or $container['houdini.log.level'] or perhaps I misunderstand the YamlConfigServiceProvider. But I am getting..

[Tue Apr 18 02:57:48 2017] PHP Fatal error: Uncaught InvalidArgumentException: Identifier "crayfish.log.level" is not defined. in /home/ubuntu/all_claw/Crayfish/Gemini/vendor/pimple/pimple/src/Pimple/Container.php:96 Stack trace: #0 /home/ubuntu/all_claw/Crayfish/Gemini/vendor/islandora/crayfish-commons/src/Provider/IslandoraServiceProvider.php(33): Pimple\Container->offsetGet('crayfish.log.le...') #1 /home/ubuntu/all_claw/Crayfish/Gemini/vendor/pimple/pimple/src/Pimple/Container.php(113): Islandora\Crayfish\Commons\Provider\IslandoraServiceProvider->Islandora\Crayfish\Commons\Provider\{closure}(Object(Silex\Application)) #2 /home/ubuntu/all_claw/Crayfish/Gemini/vendor/silex/silex/src/Silex/Provider/MonologServiceProvider.php(94): Pimple\Container->offsetGet('monolog.logfile') #3 /home/ubuntu/all_claw/Crayfish/Gemini/vendor/pimple/pimple/src/Pimple/Container.php(113): Silex\Provider\MonologServiceProvider->Silex\Provider\{closure}(Object(Silex\Application)) #4 /home/ubuntu/all_claw/Crayfish/Gemini/vendor/silex/silex/src/Silex/Provider/MonologServicePro in /home/ubuntu/all_claw/Crayfish/Gemini/vendor/pimple/pimple/src/Pimple/Container.php on line 96

So YamlConfigServiceProvider puts all the configuration from a particular YAML file into a "namespace". The default namespace is crayfish. It then loads every key in the YAML file concatenating them together with dots.

So for example:

--- test: for: jared

Would make a new entry in the service container: crayfish.entry.for. This would have a value of jared. That is why all the configuration is loading from the crayfish namespace.

So for the issue you are having here is that the crayfish.log.level variable isn't defined on the container. It should be defined here. Do you have it defined in your config file?

Yes, I have that exact same file. Other than changing the db username/password. So there is clearly something else going on.

Ok so it seemed that the issue was that IslandoraServiceProvider is trying to use the crayfish.log.level when it is instantiated. Which is before the YamlConfigServiceProvider is registered and so there are no configuration options? But that seems like it would never have worked for you.

However, if I switch the order then I get errors around the missing definiton of $app['db'].

I'll try cleaning this out and start again.

I was cleaning some things up last night at 10pm when I did the rebase and maybe I broke something. I'll give it another try now and make sure it still works for me. Sorry about that.

No worries, we should figure out how to set up a memory based backend and then we could write some functional tests to ensure these two services operate together consistently.

Did you test Houdini or Hypercube, or just Gemini? I just tried Houdini and Hypercube and they seem to be working for me. Having some DB issues with Gemini right now that I think are just VM issues, but I'm going to keep trying it.

I didn't, but I will.

Just pushed a commit that should fix the DB settings for gemini. I did break them at the last moment last night.

whikloj · 2017-04-18T03:01:48Z

src/Provider/IslandoraServiceProvider.php

+
+        // Configure external services
+        $container['monolog.logfile'] = function ($container) {
+            return strtolower($container['crayfish.log.level']) == 'none' ? null : $container['crayfish.log.file'];


I think this is the same issue as below.

whikloj · 2017-04-18T03:11:57Z

src/Provider/YamlConfigServiceProvider.php

+use Symfony\Component\Yaml\Yaml;
+use InvalidArgumentException;
+
+class YamlConfigServiceProvider implements ServiceProviderInterface


Wondering if using the Symfony ConfigurationInterface might not be preferable? Allows us to easily set defaults and do configuration validation?

http://symfony.com/doc/current/components/config/definition.html

See also @acoburn's use in static-ldp.
https://github.com/trellis-ldp/static-ldp/blob/master/src/TrellisConfiguration.php

See reply below.

jonathangreen · 2017-04-18T11:54:33Z

Not sure I understand the YamlConfigServiceProvider. I don't know where the crayfish configuration parameters would come from, and I'm not sure if I like all the configuration stuff being placed into the container along side all the actual services/providers...

Silex ServiceProviders all load their configuration from the container in the way that we do in this pull request. I believe this is the "Silex way" to load configuration.

Pimple (the service container) also has a section on storing parameters in the service container, and gives documentation about it, so its not like its an unsupported use case.

Wondering if using the Symfony ConfigurationInterface might not be preferable? Allows us to easily set defaults and do configuration validation?

In the other ServiceProviders from Silex linked above, they load their default config into the container, and then the application using the ServiceProvider is free to override the settings by adding parameters to the container, this is the pattern I followed in this pull request.

I took a look at the Symfony ConfigurationInterface when starting this pull request, and it didn't seem like a good for for how Silex does things. We chose to use Silex because it is lighter weight then a full Symfony application, and I think we should follow the patterns they use. That said I have not used ConfigurationInterface extensively so maybe I am wrong.

jonathangreen · 2017-04-18T12:03:10Z

Any other @Islandora-CLAW/committers have any opinions about this one? If we should use the Symfony ConfigurationInterface here or not?

DiegoPino · 2017-04-18T12:19:13Z

src/Provider/IslandoraServiceProvider.php

+    public function register(Container $container)
+    {
+        // Register services we rely on
+        $container->register(new MonologServiceProvider());


I think in-service registering is discouraged in Silex. It runs, so not a bug, and yes, container implements register method, but this introduces hidden dependencies for IslandoraServiceProvider breaking dependency injection possibilities (There is a lot about the topic in google). The implementer of IslandoraServiceProvider, i guess crayfish? should be responsible for registering into Pimple those services we rely on. IslandoraServiceProvider should just issue alerts/throw exceptions in case the named containers are not present/not initialized plus provide inline documentation stating that it requires those.

The alternative here is that we register each component separately in each and every microservice within crayfish, which is much harder and less testable then registering a service within a service.

The point of this provider is the provide a way to bootstrap a container for a crayfish service, so we can keep each service relatively lean and share code. I could see this being discouraged in a service meant for a general purpose, but I think it is okay here, when we know we are going to be using the service from crayfish.

I'm generally of the opinion that micro services should be configured separately. That is, while sharing configuration makes some things easier, it is usually the wrong thing to do.

I disagree on those points Jonathan but again, I won't oppose, it is just not the silex way. Testing services that don't register their own dependencies is in fact easier. I get your point that this may not a general use service, ok, but imagine the case that another service (islandora one) requires access to database under different means or another related services uses monolog under a different configuration. By obscuring the dependencies in the code, an implementer could override (in a bad for us, but in an silex expected way) what you already build here at service provider (settings etc) and since the whole silex/pimple idea is based on lazy loading, means nothing is initialized, set, called until it is used it is complicated to define upfront which of the registrations will in fact override the other one.

DiegoPino · 2017-04-18T12:20:07Z

src/Provider/IslandoraServiceProvider.php

+use Pimple\ServiceProviderInterface;
+use Silex\Provider\DoctrineServiceProvider;
+use Silex\Provider\MonologServiceProvider;
+use Silex\Provider\ServiceControllerServiceProvider;


Silex\Provider\ServiceControllerServiceProvider i not used here. ServiceProviderInterface comes from pimple

Is used right here:
https://github.com/Islandora-CLAW/Crayfish-Commons/pull/8/files#diff-557c0b35d280aea3042d482bf149d1baR27

Jonathan, sorry those are new silex patterns for me. Why do you create new instance of a class that is made to be extended and does not much by itself? No critics, but I don't get it.

I am following the documentation for the class.
https://silex.sensiolabs.org/doc/2.0/providers/service_controller.html

@jonathangreen that is only needed (at app level) to allow Controllers (route controllers, like the code that reacts to GET or POST) to be defined as services, its a way of overriding the normal controller resolver to allow classes to be used , stuff like this
$app->get('/ldp', "posts.controller:givememyJson");

I don't see that here. The documentation there is a bit misleading but if you look further down it expands to the controller notion.

The point is that this service is bootstrapping the container for other crayfish apps, so it needs to load this service so that the apps don't. Thats the whole purpose of this.

acoburn · 2017-04-18T13:05:51Z

I am in favor of using the Symfony ConfigurationInterface

whikloj · 2017-04-19T14:01:55Z

Ok, so this works for me now.

There are two issues that I think are still unresolved.

The dependency injection question of registering the services inside services.
The YamlConfigServiceProvider versus the Symfony ConfigurationInterface.

I also think the YamlConfigServiceProvider could probably be merged with IslandoraServiceProvider...but that is just my opinion.

We have a short timeline before IslandoraCon, so I'm going to merge this (and Islandora/Crayfish#27) but leave issue Islandora/documentation#603 open and add my opinions there.

I'll also open a new ticket around the "not registering services inside services" discussion.

Does that sound good to all the @Islandora-CLAW/committers?

dannylamb · 2017-04-19T14:06:24Z

Sounds great @whikloj

dannylamb · 2017-04-19T14:07:52Z

I'm also adding to the CLAW call agenda if it's not already there, so we can discuss and move forward in a way that keeps everyone happy since I don't think all concerns have really been addressed.

jonathangreen · 2017-04-19T14:32:50Z

Sounds great to me as well @whikloj. Thanks again for testing.

whikloj · 2017-04-19T16:40:16Z

@acoburn haven't heard from you, so hopefully you are okay continuing this conversation on Islandora/documentation#603 and I'll merge this in the interim.

acoburn · 2017-04-19T16:41:00Z

@whikloj yes, merge away!

ruebot reviewed Apr 17, 2017

View reviewed changes

jonathangreen added 6 commits April 17, 2017 16:00

Rename

d5f6511

Update and Add Service Providers

cddb97d

* Update IslandoraServiceProvider to be more idiomatic in the way it uses configuration variables. * Add a new YamlConfigServiceProvider to load configuration from YAML files for Crayfish services.

Update some variable names

4bf4299

Fix coding standards issues

633972d

Add tests for islandora service provider

5a8a467

Add tests for bug found in settings parser.

f65d570

jonathangreen force-pushed the issue-603 branch from d7f2116 to f65d570 Compare April 17, 2017 19:06

jonathangreen added 2 commits April 17, 2017 16:23

Update doctrine settings a bit

f43d15a

Update readme to include codecob

90dfdd8

jonathangreen mentioned this pull request Apr 17, 2017

Add YAML Config Files Islandora/Crayfish#27

Merged

whikloj reviewed Apr 18, 2017

View reviewed changes

DiegoPino reviewed Apr 18, 2017

View reviewed changes

Fix db settings.

a6b4b6e

whikloj merged commit fc3c832 into Islandora:master Apr 19, 2017

whikloj mentioned this pull request Apr 19, 2017

Configuration in Crayfish Islandora/documentation#603

Closed

jonathangreen deleted the issue-603 branch March 25, 2019 14:30

Add YAML configuration files for micro services #8

Add YAML configuration files for micro services #8

Conversation

jonathangreen commented Apr 17, 2017 • edited Loading

GitHub Issue

What does this Pull Request do?

How should this be tested?

Interested parties

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 17, 2017 • edited Loading

Codecov Report

whikloj left a 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

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

jonathangreen commented Apr 18, 2017 • edited Loading

jonathangreen commented Apr 18, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acoburn commented Apr 18, 2017

whikloj commented Apr 19, 2017

dannylamb commented Apr 19, 2017

dannylamb commented Apr 19, 2017 • edited Loading

jonathangreen commented Apr 19, 2017

whikloj commented Apr 19, 2017

acoburn commented Apr 19, 2017

jonathangreen commented Apr 17, 2017 •

edited

Loading

codecov bot commented Apr 17, 2017 •

edited

Loading

jonathangreen commented Apr 18, 2017 •

edited

Loading

jonathangreen commented Apr 18, 2017 •

edited

Loading

dannylamb commented Apr 19, 2017 •

edited

Loading