-
-
Notifications
You must be signed in to change notification settings - Fork 11
build: update to Solr 9 (latest), fixes #69 #70
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
docker-compose.solr.yaml
Outdated
SOLR_AUTH_TYPE: basic | ||
SOLR_AUTHENTICATION_OPTS: -Dbasicauth=solr:SolrRocks | ||
# needed for Drupal Search API Solr | ||
SOLR_MODULES: "extraction,langid,ltr,analysis-extras" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SOLR_MODULES
should be configurable, just like SOLR_BASE_IMAGE
, and added to the README.md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Not exactly sure about syntax for this, would
SOLR_MODULES: ${SOLR_MODULES:-extraction,langid,ltr,analysis-extras}
be adequate to make it configurable, but use the default in case?
What about the hyphen after the colon? And are escaped double quotes needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should look like:
SOLR_MODULES: ${SOLR_MODULES-extraction,langid,ltr,analysis-extras}
This way you can "unset" it with ddev dotenv set .ddev/.env.solr --solr-modules=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or it can be empty by default:
SOLR_MODULES: ${SOLR_MODULES:-}
And then post_install_actions
can check if it's Drupal, and run this command:
ddev dotenv set .ddev/.env.solr --solr-modules=extraction,langid,ltr,analysis-extras
I don't know much about solr stability, but would |
Yes seems to make great sense, tests are also getting 9.9 and it would not be hardcoded to a specific version. Though I do not know whether they will release any 9.x versions or only 10.x someday. |
README.md
Outdated
| Variable | Flag | Default | | ||
| -------- | ---- |-----------| | ||
| `SOLR_BASE_IMAGE` | `--solr-base-image` | `solr:9` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add SOLR_MODULES
to this table.
And move ### Add custom Solr libraries via SOLR_MODULES env variable
section before All customization options (use with caution):
line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, should All customization options (use with caution):
have a heading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh just recognizing that Add third party Solr modules and libraries
is somewhat outdated regarding Solr 9.8 ... (lib-directives are deprecated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, should
All customization options (use with caution):
have a heading?
I don't have heading for that in the template https://github.com/ddev/ddev-addon-template/blob/main/README_ADDON.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh just recognizing that
Add third party Solr modules and libraries
is somewhat outdated regarding Solr 9.8 ... (lib-directives are deprecated)
I'm not an expert, but if you think the wording can be improved, maybe use something like: "Use this technique if your Solr version is earlier than 9.8"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but let's get more approvals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for stepping up to this! Great contribution.
Since the modules are so important, please add test coverage for them, thanks.
SOLR_AUTH_TYPE: basic | ||
SOLR_AUTHENTICATION_OPTS: -Dbasicauth=solr:SolrRocks | ||
# needed for Drupal Search API Solr | ||
SOLR_MODULES: ${SOLR_MODULES-extraction,langid,ltr,analysis-extras} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test coverage for this, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really a noob in terms of tests. But just asked on the high level at first:
I would propose a test that creates a core based on Drupal Search API solr jump start config should succeed with the SOLR_MODULES set and fail with the SOLR_MODULES unset.
Would that be in line with what you are thinking about testing here? Or something completely different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests aren't hard, look at the existing ones and read https://github.com/ddev/ddev-addon-template and links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would test first to make sure that the default modules actually get installed in the solr container (ddev exec -s solr ...
)
Then test to see that a changed SOLR_MODULES changes what you get.
With the combination of the 0.7.3 release in August and this change, I can now successfully create collections in 9.8.1 and 9.9. Without it, I get errors on startup or with 'upload configset' in Drupal. I did not change my configset (there are some mods to use extra features in 9.6+) but they applied clean and without error, and indexing works. |
The Issue
How This PR Solves The Issue
change the build to point to solr:9.9 docker image by default
add
SOLR_MODULES: "extraction,langid,ltr,analysis-extras"
to environment (for Drupal Search API Solr)
Manual Testing Instructions
Automated Testing Overview
Release/Deployment Notes