Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client configuration #4

Merged
merged 2 commits into from Sep 27, 2022
Merged

Conversation

tg666
Copy link
Contributor

@tg666 tg666 commented Oct 19, 2021

Hi,
when integrating the extension into our project, I found out that the Client configuration has an invalid structure and also that default (null) values are getting into the Client.

Current configuration example:

contributte.elastica:
	debug: %debugMode%
	hosts:
		es01:
			host: elasticsearch
			port: 9200
<?phppublic function createServiceContributte__elastica__client(): Contributte\Elastica\Client
{
	$service = new Contributte\Elastica\Client(
		[
			'es01' => [
				'host' => 'elasticsearch',
				'port' => 9200,
				'path' => null,
				'url' => null,
				'proxy' => null,
				'transport' => null,
				'persistent' => null,
				'timeout' => null,
				'connections' => [],
				'roundRobin' => null,
				'compression' => null,
				'log' => null,
				'retryOnConflict' => null,
				'bigintConversion' => null,
				'username' => null,
				'password' => null,
			],
		],
		null,
		$this->getService('contributte.monolog.logger.default')
	);
	$this->getService('contributte.elastica.panel')->register($service);
	return $service;
}

But options for the default Client should be in the configuration root. In the case of multiple nodes, they are defined under the key connections.

New configuration example:

contributte.elastica:
	debug: %debugMode%
	config:
		host: elasticsearch
		port: 9200
public function createServiceContributte__elastica__client(): Contributte\Elastica\Client
{
	$service = new Contributte\Elastica\Client(
		['host' => 'elasticsearch`, 'port' => 9200],
		null,
		$this->getService('contributte.monolog.logger.default')
	);
	$this->getService('contributte.elastica.panel')->register($service);
	return $service;
}

I also marked options host, port, username, password and auth_type as dynamic - parameters, env variables etc. are allowed.

Package nette/schema is required because of usage of the method ::skipDefaults() that has been added in the version ^1.2.

- added dependency on package `nette/schema`
- fixed structure of config schema for the compiler extension `ElasticaExtension`
- removed an option `compression` (the option is not defined in `ClientConfiguration` class)
- removed an option `log` (the option has been removed in version `7.0`)
- added an option `auth_type`
- updated examples in README
@f3l1x
Copy link
Member

f3l1x commented Nov 2, 2021

Hi. Does it means there is not support for multiple configuration?

@tg666
Copy link
Contributor Author

tg666 commented Nov 2, 2021

Ruflin/Elastica supports multiple connections. They can be defined under key connections:
https://github.com/ruflin/Elastica/blob/master/src/Client.php#L619

For example:

contributte.elastica:
	debug: %debugMode%
	config:
		connections:
			-
				host: host1
				port: 1234
				# ...
			-
				host: host2
				port: 5678
				# ...

.docs/README.md Outdated
```
Extension does not pass any unset values to elastica so elastica defaults just work.
Take a look to [Elastica docs](https://elastica-docs.readthedocs.io/en/latest/client.html#client-configurations).

## Usage
## Usage**
Copy link
Member

Choose a reason for hiding this comment

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

Typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

'transport' => Expect::string()->nullable(),
'persistent' => Expect::bool(),
'timeout' => Expect::int()->nullable(),
'connections' => Expect::array(), // host, port, path, transport, compression, persistent, timeout, username, password, auth_type, config -> (curl, headers, url)
Copy link
Member

Choose a reason for hiding this comment

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

Here must be array of structure with predefined keys. Can you put it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

'bigintConversion' => Expect::bool(),
'username' => Expect::string()->nullable()->dynamic(),
'password' => Expect::string()->nullable()->dynamic(),
'auth_type' => Expect::string()->nullable()->dynamic(), //basic, digest, gssnegotiate, ntlm
Copy link
Member

Choose a reason for hiding this comment

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

What about enum instead of string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I changed it to an enum.

@f3l1x
Copy link
Member

f3l1x commented Nov 2, 2021

Oh, I see it now. Thanks. I've added some comments.

- options for the default (root) client and also options under a key `connections` are strictly defined in a configuration schema
- updated the configuration keys in README
'config' => Expect::structure(array_merge(
$clientConfig,
[
'connections' => Expect::arrayOf(
Copy link
Member

Choose a reason for hiding this comment

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

Why is it merged here? Can we skip merging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a client config contains 16 options and the options are present in the root config and also under the key config.connections[].
Merging came to be better for me. But yeah if you insist we can skip merging and define the options duplicated.

@@ -22,7 +22,8 @@
"php": ">=7.2",
"ruflin/elastica": "^7.0",
"nette/di": "^3.0",
"nette/utils": "^3.0"
"nette/utils": "^3.0",
"nette/schema": "^1.2"
Copy link
Member

Choose a reason for hiding this comment

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

nette/schema is tight with nette/di, we can skip it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I wrote in the first post:

Package nette/schema is required because of usage of the method ::skipDefaults() that has been added in the version ^1.2.

All versions of nette/di requires nette/schema in versions ^1.0 or ^1.1 so here can be compatibility problem.
Another solution could be to remove nette/schema from requirements and add the section conflict:

{
	"conflict": {
		"nette/schema": "<1.2"
	}
}

@f3l1x
Copy link
Member

f3l1x commented Dec 19, 2021

Ping @tg666

@jiripudil
Copy link
Member

Hi, is there anything I can help with to get this merged? Correct me if I'm wrong (please do!) but I think the package is now unusable e.g. with docker-compose where ES is usually accessible via a different hostname than the default localhost 🤔

@f3l1x f3l1x merged commit c38f760 into contributte:master Sep 27, 2022
Copy link
Member

f3l1x commented Sep 28, 2022

It's OK, thanks for pinging me. @jiripudil

f3l1x pushed a commit that referenced this pull request Sep 28, 2022
- added dependency on package `nette/schema`
- fixed structure of config schema for the compiler extension `ElasticaExtension`
- removed an option `compression` (the option is not defined in `ClientConfiguration` class)
- removed an option `log` (the option has been removed in version `7.0`)
- added an option `auth_type`
- updated examples in README
- options for the default (root) client and also options under a key `connections` are strictly defined in a configuration schema
- updated the configuration keys in README
@tg666 tg666 deleted the feature/client-configuration branch April 16, 2024 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants