Skip to content

Conversation

ehanson8
Copy link
Contributor

@ehanson8 ehanson8 commented Mar 11, 2024

Purpose and background context

Implement ADR # 3 for timdex-index-manager after transmogrifier has been updated.

How can a reviewer manually see the effects of these changes?

Ensure that TIMDEX_OPENSEARCH_ENDPOINT is not set as an env var and spin up a local Docker container with:

docker run -p 9200:9200 -p 9600:9600 -e "discovery.type=single-node" -e "plugins.security.disabled=true" opensearchproject/opensearch:2.11.0

In new terminal window, create a new gismit index:

pipenv run tim create -s gismit

Copy the index name and promote the index to the gismit alias:

pipenv run tim promote -a gismit -i <index name>

Set Dev1 credentials and bulk-index the following S3 file with publishers field to see that it successfully indexes:

pipenv run tim bulk-index -s gismit s3://timdex-extract-dev-222053980223/gismit/gismit-2024-03-11-full-transformed-records-to-index.json

Includes new or updated dependencies?

YES

Changes expectations for external applications?

YES

What are the relevant tickets?

Developer

  • All new ENV is documented in README
  • All new ENV has been added to staging and production environments
  • All related Jira tickets are linked in commit message(s)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer(s)

  • The commit message is clear and follows our guidelines (not just this PR message)
  • There are appropriate tests covering any new functionality
  • The provided documentation is sufficient for understanding any new functionality introduced
  • Any manual tests have been performed or provided examples have been verified
  • New dependencies are appropriate or there were no changes

Why these changes are being introduced:
* Implement ADR # 3 for timdex-index-manager after transmogrifier has been updated

How this addresses that need:
* Add publishers field to opensearch_mappings.json
* Remove publication_information field

Side effects of this change:
* Reindexing required for new field mapping

Relevant ticket(s):
* https://mitlibraries.atlassian.net/browse/GDT-211
@coveralls
Copy link

Pull Request Test Coverage Report for Build 8238204140

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 8102485324: 0.0%
Covered Lines: 413
Relevant Lines: 413

💛 - Coveralls

Copy link
Contributor

@ghukill ghukill 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!

Comment on lines +331 to +334
"date": {
"type": "keyword",
"normalizer": "lowercase"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this feels right, that we don't also try and parse this to a date string. Maybe we'll revisit if we extend this pattern a bit, but if we know we're already duplicating dates to Dates then this as a date field likely isn't needed. With, the benefit of handling non-normal date strings that may still contain information.

@ghukill ghukill self-requested a review March 12, 2024 18:14
@ehanson8 ehanson8 merged commit b628e09 into main Mar 12, 2024
@ehanson8 ehanson8 deleted the GDT-211-publishers-mapping branch March 12, 2024 19:12
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.

4 participants