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

Allow use of different core name, fixes #12 #14

Merged
merged 4 commits into from
Dec 31, 2022
Merged

Conversation

rfay
Copy link
Member

@rfay rfay commented Dec 30, 2022

This makes it easier to use a different core name.

From the updated README:

If you want to use a core name other than the default "dev", edit docker-compose.solr.yaml to

  1. Remove the #ddev-generated at the top of the file.
  2. Change SOLR_CORE environment variable in the environment: section.
  3. Change your Drupal configuration to use the new core.

You can test this PR with https://github.com/rfay/ddev-drupal9-solr/tarball/20221230_config_corename

@claudiu-cristea
Copy link

claudiu-cristea commented Dec 31, 2022

@rfay, I wonder how to test the work in a branch. I've tried:

$ ddev get https://github.com/drud/ddev-drupal9-solr/tree/20221230_config_corename

but it doesn't work

EDIT: Sorry, only now I saw the tarball URL :)

Copy link

@claudiu-cristea claudiu-cristea left a comment

Choose a reason for hiding this comment

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

Tested and works as expected. Just a nit regarding the Solr name spelling

README.md Outdated
@@ -24,7 +24,7 @@ This repository allows you to quickly install Apache Solr for Drupal 9 into a [D
This is the classic Drupal solr:8 recipe used for a long time by Drupal users and compatible with search_api_solr.

* It installs a [`.ddev/docker-compose.solr.yaml`](docker-compose.solr.yaml) using the solr:8 docker image.
* A standard Drupal 9 solr configuration is included in [.ddev/solr/conf](solr/conf).
* A standard Drupal 9+ solr configuration is included in [.ddev/solr/conf](solr/conf).

Choose a reason for hiding this comment

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

I think we should use the official/capitalized name for Solr s/solr/Solr

Choose a reason for hiding this comment

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

This is still not capitalized

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, got it.

  • You are welcome to do a PR to this to improve the README or anything else, but this PR was about using a different core name.
  • When you notice things like this, try to use the "suggest" option when you can, which makes it so the maintainer can just commit your suggestion.

Allow_use_of_different_core_name__fixes__12_by_rfay_·_Pull_Request__14_·_drud_ddev-drupal9-solr

Choose a reason for hiding this comment

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

@rfay, my remark was only for that occurrence, in the line that was already touched in this PR. I didn't suggest to replace all occurrences. Sorry for any trouble. But true, I'm often forgetting the "suggest" tool.

@rfay
Copy link
Member Author

rfay commented Dec 31, 2022

@claudiu-cristea thanks for reviewing, I need to ask you to do it again. I changed the approach so one can do a ddev get again without editing and removing #ddev-generated, etc.

To review

  • Remove .ddev/docker-compose.solr.yaml
  • ddev get https://github.com/rfay/ddev-drupal9-solr/tarball/20221230_config_corename
  • Add the recommended docker-compose.solr-env.yaml from the README.

@rfay rfay requested a review from claudiu-cristea December 31, 2022 13:43
Copy link

@claudiu-cristea claudiu-cristea left a comment

Choose a reason for hiding this comment

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

It works correctly by overriding env vars in docker-compose.solr-env.yaml. I've also run:

docker-compose -f ./.ddev/.ddev-docker-compose-full.yaml config solr

...(BTW, I've missed a ddev docker-compose-config command to wrap docker-compose config) to make sure the env vars are merged correctly and the vars section looks good:

services:
  solr:
    ...
    environment:
      HTTP_EXPOSE: 8983:8983
      SOLR_CORENAME: somecorename
      VIRTUAL_HOST: joinup.ddev.site

This looks good to go. Thank you!

@rfay rfay merged commit 0440d2b into main Dec 31, 2022
@rfay rfay deleted the 20221230_config_corename branch December 31, 2022 14:02
@rfay
Copy link
Member Author

rfay commented Dec 31, 2022

Thanks so much @claudiu-cristea - since you use this your care for it is appreciated. There's still a need for getting @mkalkbrenner's preferred technique into (another) add-on. based on ddev/ddev-contrib#195

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.

2 participants