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

global: add support for index templates #148

Merged
merged 3 commits into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/actions/pre-install/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
runs:
using: composite
steps:
- uses: ts-graphviz/setup-graphviz@v1
53 changes: 5 additions & 48 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,57 +7,14 @@ on:
branches: master
schedule:
# * is a special character in YAML so you have to quote this string
- cron: '0 3 * * 6'
- cron: "0 3 * * 6"
workflow_dispatch:
inputs:
reason:
description: 'Reason'
description: "Reason"
required: false
default: 'Manual trigger'
default: "Manual trigger"

jobs:
Tests:
runs-on: ubuntu-20.04
strategy:
fail-fast: false
matrix:
# You can add/remove combinations e.g. `dev` requirements or `postgresql13` by adding
# a new item to the following lists.
# You can see the complete list of services and versions that are available at:
# https://docker-services-cli.readthedocs.io/en/latest/configuration.html
python-version: [3.9, 3.12]
search-service: [opensearch2, opensearch1]
include:
- search-service: opensearch2
SEARCH_EXTRAS: "opensearch2"

- search-service: opensearch1
SEARCH_EXTRAS: "opensearch1"

env:
SEARCH: ${{ matrix.search-service }}
EXTRAS: tests,${{ matrix.SEARCH_EXTRAS }}

steps:
- name: Checkout
uses: actions/checkout@v4

- name: Setup Graphviz
uses: ts-graphviz/setup-graphviz@v1

- name: Set up Python ${{ matrix.python-version }}
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
cache: pip
cache-dependency-path: setup.cfg

- name: Install dependencies
run: |
pip install ".[$EXTRAS]"
pip freeze
docker --version
docker-compose --version

- name: Run tests
run: ./run-tests.sh
Python:
uses: inveniosoftware/workflows/.github/workflows/tests-python.yml@master
7 changes: 7 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@
Changes
=======

Version 4.1.0 (release 2024-08-14)
----------------------------------

- introduce a new config `STATS_REGISTER_INDEX_TEMPLATES` to be able to register
events and aggregations as index templates (ensure backwards compatibility)


Version 4.0.2 (release 2024-03-04)
----------------------------------

Expand Down
2 changes: 1 addition & 1 deletion invenio_stats/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ def register_queries():
from .ext import InvenioStats
from .proxies import current_stats

__version__ = "4.0.2"
__version__ = "4.1.0"

__all__ = (
"__version__",
Expand Down
6 changes: 6 additions & 0 deletions invenio_stats/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,9 @@
delivery_mode="transient", # in-memory queue
)
"""Default exchange used for the message queues."""

STATS_REGISTER_INDEX_TEMPLATES = False
"""Register templates as index templates.

Default behaviour will register the templates as search templates.
"""
20 changes: 18 additions & 2 deletions invenio_stats/templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@

"""Celery background tasks."""

from flask import current_app

from .proxies import current_stats


def register_templates():
"""Register search templates for events."""
def _collect_templates():
"""Return event and aggregation templates from config."""
event_templates = [
event["templates"] for event in current_stats.events_config.values()
]
Expand All @@ -21,3 +23,17 @@ def register_templates():
]

return event_templates + aggregation_templates


def register_templates():
"""Register search templates for events."""
if current_app.config["STATS_REGISTER_INDEX_TEMPLATES"]:
return []
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it makes sense to show a deprecation warning without having a "way out". Usually these warnings link to some documentation that explains why the change is happening and how to migrate to the new solution (see example below).

I think to move things forward it would be better to cut a minor release, and include this deprecation warning in a new PR that would also include this extra information in the text (and release as a minor/patch).


GH Actions deprecation warning

image

SQLAlchemy

SADeprecationWarning: When transforming <class '__main__.User'> to a
dataclass, attribute(s) "create_user", "update_user" originates from
superclass <class
'__main__.Mixin'>, which is not a dataclass. This usage is deprecated and
will raise an error in SQLAlchemy 2.1. When declaring SQLAlchemy
Declarative Dataclasses, ensure that all mixin classes and other
superclasses which include attributes are also a subclass of
MappedAsDataclass.

return _collect_templates()


def register_index_templates():
"""Register search index templates for events."""
if not current_app.config["STATS_REGISTER_INDEX_TEMPLATES"]:
return []
Copy link
Contributor

Choose a reason for hiding this comment

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

these functions look a bit too similar and I am worried they will be easily confusing, but I don't have a good idea on how to avoid this. Maybe a bit more elaborate comments on both to explain the backwards compatibility of register_templates vs register_index_templates?
also I guess, we cannot raise errors instead of empty list, because we lose the backwards compatibility ?

return _collect_templates()
2 changes: 2 additions & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ invenio_base.api_blueprints =
invenio_stats = invenio_stats.views:blueprint
invenio_search.templates =
invenio_stats = invenio_stats.templates:register_templates
invenio_search.index_templates =
Copy link
Contributor

Choose a reason for hiding this comment

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

any way of installing one or the other, not both? maybe we need to deprecate invenio_search.templates?

invenio_stats = invenio_stats.templates:register_index_templates
invenio_queues.queues =
invenio_stats = invenio_stats.queues:declare_queues

Expand Down
Loading