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

make valkyrie solr adapter take full url argument #6300

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Conversation

orangewolf
Copy link
Member

Summary

Previously Valkyrie solr could only be set with host, port and core. This prevents Valkyrie from being able to work with authenticated Solr or really any Solr that doesn't follow the strict pattern. Long term, I feel like the Valkyrie config should go away and we should just support the same options as the blacklight.yml already supports. Specifying Solr twice seems unnecessary, but this at least provides a solution which is good enough for production.

Guidance for testing, such as acceptance criteria or new user interface behaviors:

  • Set a config/valkyrie_index.yml that looks like
development:
  adapter: solr
  url: <%= ENV['SOLR_URL'] + 'hydra-development' || "http://127.0.0.1:#{ENV.fetch('SOLR_DEVELOPMENT_PORT', 8983)}/solr/hydra-development" %>
test: &test
  adapter: solr
  url: <%= ENV['SOLR_URL'] ? ENV['SOLR_URL'] + 'hydra-test' : "http://127.0.0.1:#{ENV.fetch('SOLR_TEST_PORT', 8985)}/solr/hydra-test" %>
production:
  adapter: solr
  url: <%= ENV['SOLR_URL'] || "http://127.0.0.1:8983/solr/blacklight-core" %>
  • Set SOLR_URL and start the app
  • It should be able to see Solr

@samvera/hyrax-code-reviewers

@orangewolf orangewolf added the notes-bugfix Release Notes: Fixed a bug label Sep 7, 2023
Copy link
Contributor

@eliotjordan eliotjordan left a 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. 👍

Copy link
Contributor

@dlpierce dlpierce left a comment

Choose a reason for hiding this comment

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

Why not use the blacklight config's url directly now? Is there a good reason to not use the same solr config for both?

@orangewolf
Copy link
Member Author

@dlpierce not from my perspective - but I felt like it was presumptuous since someone did work to get it to behave this way. @no-reply do you have reasons for them being seperate?

@dlpierce
Copy link
Contributor

dlpierce commented Sep 7, 2023

I'll point out koppie has been configured to use the same core in both config files.

@no-reply
Copy link
Contributor

the idea was that one might want to run Blacklight against the index/core being written to by AF, and index valkyrie records elsewhere until the index was caught up, then change the blacklight config.

in principle, i don't think it's nice to make it too hard to separate the configurations. we're configuring two different things, so i should have the option to configure them separately if that's convenient for me. but i don't see any reason the default shouldn't be to use the blacklight config.

@no-reply no-reply merged commit cc555cb into main Sep 19, 2023
4 checks passed
@no-reply no-reply deleted the valkyrie_solr_url branch September 19, 2023 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-bugfix Release Notes: Fixed a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants