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

Feature/4016 anon import aliases #2433

Merged
merged 7 commits into from
Nov 28, 2024

Conversation

philipkcl
Copy link
Contributor

@philipkcl philipkcl commented Nov 20, 2024


anon import aliases

briefly describe the PR here

This PR...

  • has scripts to run
  • has migrations to run
  • adds new infrastructure
  • changes the CI pipeline
  • affects the public site
  • affects the editorial area
  • affects the publisher area
  • affects the monitoring

Developer Checklist

Developers should review and confirm each of these items before requesting review

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Reviewer Checklist

Reviewers should review and confirm each of these items before approval
If there are multiple reviewers, this section should be duplicated for each reviewer

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Testing

List user test scripts that need to be run

List any non-unit test scripts that need to be run by reviewers

Deployment

What deployment considerations are there? (delete any sections you don't need)

Configuration changes

What configuration changes are included in this PR, and do we need to set specific values for production

Scripts

What scripts need to be run from the PR (e.g. if this is a report generating feature), and when (once, regularly, etc).

Migrations

What migrations need to be run to deploy this

Monitoring

What additional monitoring is required of the application as a result of this feature

New Infrastructure

What new infrastructure does this PR require (e.g. new services that need to run on the back-end).

Continuous Integration

What CI changes are required for this

Copy link
Contributor

@Steven-Eardley Steven-Eardley left a comment

Choose a reason for hiding this comment

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

This is a great improvement, thanks @philipkcl - just a couple of changes suggested before merge.

@@ -964,6 +966,25 @@ def refresh():
return ES.indices.refresh()


def find_indexes_by_prefix(index_prefix) -> list[str]:
data = ES.indices.get(f'{index_prefix}*')
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be good to enforce the separator here (if not index_prefix.endswith('-') ...) because here is where a missing separator could match a whole lot more indexes than we indend. E.g. if you supply doaj as the index prefix you'll delete indexes doajstatic-article etc. That won't happen unless the comment in settings.py to include the separator is ignored.

Perhaps we should just ensure the setting always ends with - instead. Either way, not a blocker for release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not recommend it.

It will fail the first time import. The first time import meaning when the the index name still calling doaj-account instead of doaj-account-20111212, which mean it will help to migrate if the environment still using old index name doaj-account.

Moreover, prefix search by index_type and the script will show which index will be removed, it should be safe enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added some condition check to avoid future index like doaj-account_abcde

portality/scripts/anon_import.py Outdated Show resolved Hide resolved
@Steven-Eardley Steven-Eardley merged commit 920a70d into develop Nov 28, 2024
2 checks passed
@Steven-Eardley Steven-Eardley deleted the feature/4016_anon_import_aliases branch November 28, 2024 12:26
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