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

Convert ensureindexfilestask to migration #3065

Conversation

ReeceHoffmann
Copy link
Member

@ReeceHoffmann ReeceHoffmann commented May 24, 2024

Changes

  • Move functionality of EnsureFileIndexTask into a migration
    • Replicates functionality of EnsureFileIndex in new migration re-using much of the previous code
    • updates test to work with
    • Adds data_path to the migration context object.
      • CLI options already existed for getting the data path but were previously ignored
  • Removes usage of EnsureFileIndexTask from data faker

Compatibility

We need to be very careful to identify breaking changes in.

Sources of breaking changes:

API

  • Removing a field from a response from an API endpoint.
  • Changing the shape of a response from an API endpoint.
  • Changing paths or search query parameters.
  • Removing deprecated functionality.
  • Any changes I have made to the API are backwards compatible.

Migration

  • Making changes that require a certain migration to have been applied.
  • My changes do not require a newer migration than the currently required migration..

Configuration

  • Changing configuration options that could break configurations in
    production and development environments.
  • My changes do not change configuration value names or types.

NOTE: The data path variable in the CLI was previously ignored. No new CLI options were technically added,
but in practice new arguments will need to be passed if the default option for data storage is not correct

  • Dev already passes the data_path as a env variable

Services

  • Making changes that require a certain version of a service like Postgres, Redis, or
    OpenFGA.
  • My changes don't impose any new service requirements.

Notes

If you have introduced breaking changes, explain here.

Pre-Review Checklist

  • All changes are tested.
  • All touched code documentation is updated.
  • Deepsource issues have been reviewed and addressed.
  • Comments and print statements have been removed.

@ReeceHoffmann ReeceHoffmann marked this pull request as ready for review May 27, 2024 18:35
Copy link
Member

@igboyes igboyes left a comment

Choose a reason for hiding this comment

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

Change typing to use literals (eg. Dict -> dict).

Looks good otherwise.

@igboyes igboyes merged commit 0c2d5f8 into virtool:main May 28, 2024
6 of 7 checks passed
@virtool-bot
Copy link
Contributor

🎉 This PR is included in version 25.0.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants