GS-8481/TTO-178: Fix missing indicators in MARC records in OAI; update dependencies & actions #27
Conversation
| solr-sdr-catalog: | ||
| condition: service_healthy | ||
| mariadb: | ||
| condition: service_healthy | ||
|
|
||
| test: |
There was a problem hiding this comment.
@Ronster2018 Note that the test service here is almost entirely duplicative of the web one except for the command. Do you have any suggestions for reducing that duplication? One option would be to just remove the test service entirely and specify docker compose run web bundle exec rspec in .github/workflows/tests.yml, but we don't really have any consistency/standards around this...
There was a problem hiding this comment.
@aelkiss This could be a good spot to use Aliases and Anchors. Fortunately it's something built into YAML and I found some documentation about it on the official docker website. https://docs.docker.com/compose/compose-file/10-fragments/. It's not something I have used extensively but it's a solid method for reducing repetition in the same file.
There was a problem hiding this comment.
Great, I'll take a look and give it a try here. I've seen this YAML construct in Rails configuration before for example https://github.com/hathitrust/otis/blob/main/config/database.yml but not written any YAML that uses it. It's good to know this is a supported feature of the compose specification rather than just something part of YAML that may or may not really be supported as part of compose.
There was a problem hiding this comment.
@Ronster2018 I gave this a try with fragments and extensions; see what you think. I'm also curious @moseshll what you think of this (see https://docs.docker.com/compose/compose-file/10-fragments/, also https://docs.docker.com/compose/compose-file/11-extension/)
docker-compose.yml
Outdated
|
|
||
| solr-sdr-catalog: | ||
| image: ghcr.io/hathitrust/catalog-solr-sample | ||
| ports: | ||
| - "9033:9033" | ||
| healthcheck: | ||
| test: [ "CMD", "/usr/bin/curl", "-s", "-f", "http://localhost:9033/solr/catalog/admin/ping"] | ||
| interval: 5s |
There was a problem hiding this comment.
@Ronster2018 I think the healthcheck built-in option is preferable to using wait-for (https://github.com/eficode/wait-for) everywhere, but including all the timing parameters does make the docker-compose.yml harder to read. Any thoughts there?
| @@ -60,7 +60,7 @@ def field974_to_field856(field974) | |||
| # @param [Array] | |||
| def add_field(full_marc, tag, subfield_codes) | |||
| full_marc.each_by_tag(tag) do |field| | |||
| new_field = MARC::DataField.new(field.tag, field["ind1"], field["ind2"]) | |||
| new_field = MARC::DataField.new(field.tag, field.indicator1, field.indicator2) | |||
There was a problem hiding this comment.
@moseshll This is the only substantive change; note the test below
aelkiss
force-pushed
the
GS-8481-missing-indicators
branch
3 times, most recently
from
938c8ce to
b5e68d8
Compare
|
Unfortunately because I renamed the action and there is no build.yaml on main, I will need to wait to test the action until we merge to main, at which point I can manually deploy to staging. |
* Use build workflow from hathitrust/otis#209 * Use multi-stage docker build rather than Dockerfile.prod * Remove deployment workflows * Update to ruby 3.3, sinatra 4 * Remove wait-for and use health checks via docker compose * Change path for validate-cache (see hathitrust/feed@22bfa8c)
aelkiss
force-pushed
the
GS-8481-missing-indicators
branch
from
b5e68d8 to
b09c840
Compare
|
Also @moseshll I did at least give this a try in development and verify that it started correctly and that MARC fields that were previously missing indicators have them now. |
netcat I think was only for wait-for and libxerces is only for validate-cached which we use only in tests
aelkiss
changed the title
There was a problem hiding this comment.
All of the changes to the Dockerfile, docker-compose.yml, and the actions look good to me and the necessary changes have been made. I believe we are just waiting on @moseshll to review a few things. Adding my stamp of approval.
There was a problem hiding this comment.
A bit of fiddling around until I found the right combination of DOCKER_DEFAULT_PLATFORM=linux/amd64 incantations. (For some reason I had to run the amd64 version of feed from a separate repo -- or at least I was able to make things work after trying that.)
I'd like to try getting feed a little more tractable on ARM if it will remain a dependency in the medium to long term.
Edit: I should have said, I am happy with the changes that were made for this PR.
This PR addresses the bug reported at https://hathitrust.atlassian.net/browse/GS-8481 - we were not correctly transferring the "indicator" field from the input MARC record to the version we output. I added a test to address this. This is covered in the first commit. @moseshll I think this part is the main thing you need to look at.
I took the opportunity to update dependencies, Dockerfile, docker-compose, and github actions to something I believe is closer to our current "state of the art". In particular this:
Along with this, there are a bunch of containerization & CI updates:
Dockerfile.prodand uses a multi-stage build to achieve the same thingdocker composerather than relying onwait-for.