-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Add run_id_pattern search for Dag Run API. #52437
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
Add run_id_pattern search for Dag Run API. #52437
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If something in UI changes it is cool to always also add a screenshot - that makes review easier. Can you add some mugshot to PR description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not fully sure about this feature, it is really a gap from previous version but the implementation only offers an exact match. No globbing.
I'd request some other reviewers as well, I assume next level is also we need to invest into better search capabilities in general, only exact run ID match is not much of a great value.
Please do not treat it tooo negative. I think in general the search capabilties are a shortcoming in the UI in general. I am unsure about the increments needed to get back to 2.11 state of options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for the PR, just a small nit. LGTM on API side after fix.
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py
Outdated
Show resolved
Hide resolved
That was fast! Awesome work. There are a number of other fields that could be sorted on, and I agree with
However, I do also think something is better than nothing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request, that's a good idea.
We do support searching on patterns:
DESCRIPTION = (
"SQL LIKE expression — use `%` / `_` wildcards (e.g. `%customer_%`). "
"Regular expressions are **not** supported."
)
You can take a look at search_param_factory
to construct such filters on the API side.
For instance for the task_display_name_pattern
search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, just a few nitpicks and should be good to merge.
Thanks for the pull request.
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py
Outdated
Show resolved
Hide resolved
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One remark
7e2987d
to
d22c187
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small nits and we should be good to merge.
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py
Outdated
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml
Outdated
Show resolved
Hide resolved
Also I think you took example on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks
sorry for bothering you for the final step
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Also for staying on it with that amount of feedback! Hope we did not scare you in reviews.
LGTM!
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Great work! |
* Add run_id filter for dag runs * Update i18n support for run_id filter in Dag Runs page * Apply param dependency injection and update i18n key structure * Apply _SearchParam to support pattern search * Rename run_id to run_id_pattern and refactor DagRuns ui to use SearchBar * Resolve merge conflict * Improve test cases * Resolve test failure * Fix: resolve Advanced Search button visibility * Improve filter layout and variable consistency
* Add run_id filter for dag runs * Update i18n support for run_id filter in Dag Runs page * Apply param dependency injection and update i18n key structure * Apply _SearchParam to support pattern search * Rename run_id to run_id_pattern and refactor DagRuns ui to use SearchBar * Resolve merge conflict * Improve test cases * Resolve test failure * Fix: resolve Advanced Search button visibility * Improve filter layout and variable consistency
nice work!! |
Feature
run_id
pattern filter to/dag_runs
pageDiscussion points
run_id
filter sufficient? or should we support more complex search parser?Feedback and suggestions are welcome!
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in airflow-core/newsfragments.