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

Add elastic8 support #253

Merged
merged 6 commits into from
Jan 5, 2025
Merged

Add elastic8 support #253

merged 6 commits into from
Jan 5, 2025

Conversation

blackshadev
Copy link
Collaborator

@blackshadev blackshadev commented Dec 20, 2024

This has one two breaking changes:

  • You can only use elastic 8 with this code, the elastic client is not backwards compatible
  • The selector namespace has changed, I updated the docs as such.

TODO

  • Test it on an elastic 8 engine
  • Update changelog

@david9801
Copy link

@blackshadev I have this issue when trying to persist a model in the database that uses the "Searchable" trait. Is this the problem you say you solve? "Driver [explorer] not supported."
I use laravel/scout 10.x & jeroen-g/explorer 3.x in laravel 11

@blackshadev
Copy link
Collaborator Author

@blackshadev I have this issue when trying to persist a model in the database that uses the "Searchable" trait. Is this the problem you say you solve? "Driver [explorer] not supported." I use laravel/scout 10.x & jeroen-g/explorer 3.x in laravel 11

No. This PR will not solve any problem. It is a upgrade to elastic 8

@Jeroen-G
Copy link
Owner

Looks pretty good, did some more cleaning up as well. Only could not yet get the demo app working (merged the PR and main should be on Laravel 11 now).

@blackshadev
Copy link
Collaborator Author

blackshadev commented Dec 26, 2024

Looks pretty good, did some more cleaning up as well. Only could not yet get the demo app working (merged the PR and main should be on Laravel 11 now).

Any logs from the elastic stack to share.

I have some reproduction steps for you.

  1. I just noted I did not did any changes to the .env.example you should add the contents below to at least your .env, docker compose uses it to set some basic vars among the elastic stack (included it in this MR along with package updates)
ES_STACK_VERSION=8.17.0
ELASTIC_PASSWORD=secret
KIBANA_PASSWORD=secret
  1. You need to remove the old volumes and containers to ensure it will start fresh
  2. You can check the logs of the setup container it will get stuck a while at Waiting for Elasticsearch availability until the es01 container is booted. If it continues to All done! es01 is fully booted and setup.
    1. If it doesn't get past Waiting for Elasticsearch availability check the logs of es01, it should contain some hints. the setup container will continue to try and log in, so you may see these attempts in the logs.
  3. If kibana doesn't fully boot, just sail up -d again. On my machine the first boot es01 was to slow to setup and thus kibana's first boot failed. After es01 was booted and I did sail up -d again it worked
  4. Ensure you get the elastic8 branch connected

I tried your branches but I couldn't get composer install to work. Did you have the same problem?

@blackshadev
Copy link
Collaborator Author

blackshadev commented Dec 26, 2024

Looks pretty good, did some more cleaning up as well. Only could not yet get the demo app working (merged the PR and main should be on Laravel 11 now).

After allot of trail and error. This is the composer.json with which I could install laravel 11 on the demo app.

I did end up needing to remove the fruitcake cors package (it should be build in now). And I needed to upgrade phpunit :(

See this MR

"require": {
        "php": "^8.1||^8.2",
        "guzzlehttp/guzzle": "^7.0.1",
        "jeroen-g/explorer": "dev-elastic8",
        "laravel/framework": "^11.0",
        "laravel/horizon": "^v5.30.1",
        "laravel/scout": "^v10.11.9",
        "laravel/tinker": "^v2.10.0"
    },
    "require-dev": {
        "spatie/laravel-ignition": "^2.1.3",
        "fakerphp/faker": "^1.9.1",
        "jeroen-g/laravel-packager": "^2.10.0",
        "laravel/sail": "^v1.39.1",
        "mockery/mockery": "^1.4.2",
        "nunomaduro/collision": "^v8.5.0",
        "phpunit/phpunit": "^11.5.0"
    },

@drmmr763
Copy link

Thank you for these efforts

@drmmr763
Copy link

Two things I have added to my provider you may want to add to your builder in ElasticClientBuilder


 // set default retries
            $client->setRetries(config()->get('explorer.connection.retries'));

// enable logger
            if (config('explorer.logging')) {
                $logger = $app->make('log');
                $client->setLogger($logger);
            }

@blackshadev
Copy link
Collaborator Author

Two things I have added to my provider you may want to add to your builder in ElasticClientBuilder


 // set default retries
            $client->setRetries(config()->get('explorer.connection.retries'));

// enable logger
            if (config('explorer.logging')) {
                $logger = $app->make('log');
                $client->setLogger($logger);
            }

I'd like to keep this PR focussed on soly the upgrade, while not adding any features. If this is merged we van add it no problem. But at least this way there is a elastic-8 version out there as fast as possible.

@drmmr763
Copy link

Understood. Did you confirm the logger works? I think that's an existing feature, this was the only way I was able to keep backwards compatible.

@blackshadev
Copy link
Collaborator Author

Understood. Did you confirm the logger works? I think that's an existing feature, this was the only way I was able to keep backwards compatible.

Didn't double check, but it should work fine.

According to ours docs you need to set logging=true and logger to either a string reference to the name of the Laravel log channel or the actual logger instance. With your suggestion we lose the logger functionality.

@Jeroen-G Jeroen-G merged commit 975cf7e into master Jan 5, 2025
11 checks passed
@Jeroen-G Jeroen-G deleted the elastic8 branch January 5, 2025 19:18
@Jeroen-G
Copy link
Owner

Jeroen-G commented Jan 5, 2025

Shipping time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants