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

Set geonames usernames using environment variable #6159

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Conversation

eliotjordan
Copy link
Contributor

@eliotjordan eliotjordan commented Aug 22, 2023

Fixes

Closes #5799 Closes #5800

Summary

Make geonames username configurable with environment variable.

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

  • To test locally, set docker-compose GEONAMES_USERNAME environment variable to valid user name
  • To test in QA, set GEONAMES_USERNAME environment variable to valid user name

Type of change (for release notes)

  • notes-minor New Features that are backward compatible

@samvera/hyrax-code-reviewers

@eliotjordan eliotjordan added the notes-minor Release Notes: Non-breaking features label Aug 22, 2023
@eliotjordan
Copy link
Contributor Author

@dlpierce To test this in nurax pg, someone will have to set an ENV var to the existing geonames username.

@@ -17,6 +17,7 @@ services:
- .koppie/.env
environment:
- RAILS_ROOT=/app/samvera/hyrax-webapp
- GEONAMES_USERNAME=username
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- GEONAMES_USERNAME=username

Let's not set a (likely unusable) default 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.

I think having a placeholder is a good idea for local development. Knowing where to put the value seems useful The application behaves the same way with a dummy value vs. no value. Perhaps changing it to your_geonames_username?

If the consensus is to remove it, I'll do it.

Copy link
Contributor Author

@eliotjordan eliotjordan Aug 24, 2023

Choose a reason for hiding this comment

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

@dlpierce I update the dummy GEONAMES_USERNAME value as described above. Thoughts? Still remove this from docker-compose?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm generally negative on dummy values in configuration and, at least last time i was up-to-date, hyrax doesn't include them in general.

ultimately i'm fine either way, but if there's going to be a dummy value, i'd think it should go in the .env, rather than the docker-compose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove it and update https://github.com/samvera/hyrax/wiki/Hyrax-Management-Guide#geonames unless there's some other place that this should be documented.

Copy link
Contributor Author

@eliotjordan eliotjordan Aug 24, 2023

Choose a reason for hiding this comment

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

Config removed from docker-compose files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update the wiki once this work is merged.

@dlpierce
Copy link
Contributor

I've added a dedicated geonames account to CircleCI's ENV config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-minor Release Notes: Non-breaking features
Projects
None yet
3 participants