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.

Sign up for GitHub

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

Jump to bottom

Trigger Button - Implement Part 2 of AIP-50 #31583

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Trigger Button - Implement Part 2 of AIP-50 #31583

merged 1 commit into from
Jun 1, 2023

Conversation

jscheffl
Copy link
Contributor

closes: AIP-50 Project
related: #31301

This PR implements Part 2 of AIP-50 and once merged closes the AIP-50 improvement.

In this PR the behavior of the Trigger Button is changed to Option B as discussed in https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-50+Trigger+DAG+UI+Extension+with+Flexible+User+Form+Concept and voted in https://lists.apache.org/thread/4dkbwob1wyl3xjbqdsmbd1mvgzflzp1f

Changes in this PR:

  • Trigger button will route to the Trigger UI form only if DAG has parameters, else it skips form display
  • Headline in trigger UI form adds a link back to DAG (I was strongly missing a way back :-D))
  • Route/endpoint of trigger UI was made consistent to other routes in DAG

Note: This code touches also parts from PR #31301 - PR #31301 should be merged and this revised before merge

How to test?

  • Start the Web UI and click on the "Trigger DAG" button in the DAG "example_params_ui_tutorial" - the form should be displayed. Trigger once to see that it still works
  • Start the Web UI and click on the "Trigger DAG" button in the DAG "dataset_consumes_1" - should start directly w/o displaying the form. A flash indicator should show that start was made.

[ ] Merge PR #31301 before this one.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels May 28, 2023
@pierrejeambrun pierrejeambrun added the AIP-50 Trigger DAG UI user Form label May 29, 2023
pierrejeambrun
pierrejeambrun reviewed May 29, 2023
View reviewed changes
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Overall LGTM, tested locally and working nicely :)

airflow/www/views.py
@@ -1963,6 +1962,7 @@ def trigger(self, session: Session = NEW_SESSION):
form_fields[k]["schema"]["custom_html_form"] = Markup(
form_fields[k]["schema"]["custom_html_form"]
)
ui_fields_defined = any("const" not in f["schema"] for f in form_fields.values())
Copy link
Member

@pierrejeambrun pierrejeambrun May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this check is not self explanatory. Took me a while to realize that we are looking for at least 1 field that is not a constant to display the UI form. Maybe a comment could help, or directly checking against the dag.params values. (Here we check against UI elements that will potentially not be displayed to know if we should display them, also we construct all of these constants UI elements to no display them, which is not great)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. I assume once PR #31301 is merged I need to resolve conflicts as #31301 is touching the same area. I'd integrate a comment at time of merge.
As PR #31301 is cleaning up code/refactoring I'd like to merge this first.

Copy link
Contributor Author

@jscheffl jscheffl Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Added the comment now to the other open PR for better understanding in code in https://github.com/apache/airflow/pull/31301/files#diff-917b70d2661fae8322c538a4f60b9d4ba755556036fe0c527eb0411a6ac60f2eR2011

@pierrejeambrun pierrejeambrun merged commit 3bcaf3d into apache:main Jun 1, 2023
@jscheffl
Copy link
Contributor Author

jscheffl commented Jun 1, 2023

Oh, the PR is now merged a bit too early :-(
Positive side effects:

Will this change then also move into 2.6.2 or be in 2.7.0? How are changes/fixes selectively applies to maintenance versions?

@pierrejeambrun
Copy link
Member

pierrejeambrun commented Jun 1, 2023 •
edited
Loading

Oh yes, right, forgot the 'merge 31301 before' part :(.

I believe this is a small enough UI only change to be incorporated in 2.6.2 in 'miscellanous' section. We often have such quality of life UI improvement in patch release. (even if strictly speaking those could be considered as improvement).

Marking this for 2.6.2, also it will help not delaying 31301.

Sorry for the inconvenience

@pierrejeambrun pierrejeambrun added this to the Airflow 2.6.2 milestone Jun 1, 2023
@jscheffl jscheffl mentioned this pull request Jun 1, 2023
Merged
@eladkal eladkal added the type:improvement Changelog: Improvements label Jun 8, 2023
eladkal pushed a commit that referenced this pull request Jun 8, 2023
@jscheffl
Trigger Button - Implement Part 2 of AIP-50 (#31583)
2e1a06a 
(cherry picked from commit 3bcaf3d)
@eladkal eladkal modified the milestones: Airflow 2.6.2, Airflow 2.7.0 Jun 9, 2023
@hussein-awala
Copy link
Member

How to trigger a Dag w/ config after this PR? I cannot access the config UI if my dag doesn't have params, is it an expected behavior?

@hussein-awala hussein-awala mentioned this pull request Jul 26, 2023
Closed
2 tasks
@jscheffl
Copy link
Contributor Author

How to trigger a Dag w/ config after this PR? I cannot access the config UI if my dag doesn't have params, is it an expected behavior?

Yes, it was expected. Was discussed in devlist and voted (with not much response but at least some) in https://lists.apache.org/thread/4dkbwob1wyl3xjbqdsmbd1mvgzflzp1f
Rationale: If no params defined it does not really make sense to ask user for them.

@hussein-awala
Copy link
Member

Thank you for the clarification.

IMHO this is almost a breaking change, DagRun conf is not deprecated, and a lot of users still use it to parameterize their dag runs without defining the params. Removing the only way to provide the conf dict via the UI is a blocker for these users.

Even if we still can provide conf via the REST API and the python API, most of the users use the UI to manage Airflow DAGs and DagRuns.

@ephraimbuddy ephraimbuddy added type:new-feature Changelog: New Features and removed type:improvement Changelog: Improvements labels Aug 2, 2023
@vatsrahul1001 vatsrahul1001 mentioned this pull request Aug 12, 2023
Closed
2 tasks
This was referenced Aug 12, 2023
Merged
Closed
@jscheffl jscheffl deleted the feature/aip-50-part-2-trigger-button branch October 28, 2023 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierrejeambrun pierrejeambrun pierrejeambrun approved these changes

@ryanahamilton ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

@ashb ashb Awaiting requested review from ashb ashb is a code owner

@bbovenzi bbovenzi Awaiting requested review from bbovenzi bbovenzi is a code owner

Assignees
No one assigned
Labels
AIP-50 Trigger DAG UI user Form area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:new-feature Changelog: New Features
Projects
None yet
Milestone
Airflow 2.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jscheffl @pierrejeambrun @hussein-awala @ephraimbuddy @eladkal