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

Run Trigger Page: Configurable number of recent configs #36878

Merged
merged 2 commits into from
Jan 20, 2024

Conversation

AchimGaedkeLynker
Copy link
Contributor

Some users are heavily relying on running DAGs manually by altering or repeating previous configurations.
The number (more precisely limit) of previous runs sent to the trigger.html selector is 5 and quite low for this use-case.

This PR makes the number of recent DAG run configurations configurable.

Questions:

  • Are there any unit tests required here?
  • I (speaking for the in house users) would love that feature sooner than later, I've put version 2.9.0 into the configuration YML, I believe that's the earliest when sticking to semantic versioning... and the change is liked by the airflow community...

^ 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 newsfragments.

Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

It would be great if you can add the test cases for this new change. If existing test cases are not available due to feasibility, then I would like to hear from others.
Can you attach the screenshot for the affected form/page?

@AchimGaedkeLynker
Copy link
Contributor Author

I'd be really happy getting advice on unit tests, figuring out the test/CI setup for each project is always daunting.

This PR seems to have a decent example for unit-testing configurations. - @dirrao , what's your take?
https://github.com/apache/airflow/pull/35460/files#diff-603905684cc5476d24f0294f961251bbb9852f881803c52bedaeb220a82db11d

@AchimGaedkeLynker
Copy link
Contributor Author

Screenshot as requested - it is the number of entries in the "Select Recent Configurations" dropdown

image

@eladkal eladkal requested a review from jscheffl January 20, 2024 07:50
Copy link
Contributor

@jscheffl jscheffl left a comment

Choose a reason for hiding this comment

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

Without major rework of the overall code in the views it is hard to add tests - also other options with parameters are not explicitly tested. Also other parameters like show_trigger_form_if_no_params are not tested.

I assume it is good like this.

As it is a new feature and not a bug I'd not merge it into a patch release - but 2.9.0 is good as well.

THANKS for the contribution!

@jscheffl jscheffl added type:improvement Changelog: Improvements area:UI Related to UI/UX. For Frontend Developers. labels Jan 20, 2024
@jscheffl jscheffl added this to the Airflow 2.9.0 milestone Jan 20, 2024
@jscheffl jscheffl merged commit 47980c4 into apache:main Jan 20, 2024
55 checks passed
flacode pushed a commit to flacode/airflow that referenced this pull request Jan 22, 2024
* Run Trigger Page: Configurable number of recent configs
* no abbreviations in configuration names
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
* Run Trigger Page: Configurable number of recent configs
* no abbreviations in configuration names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants