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

Use default view in dr op link #11778

Merged

Conversation

turbaszek
Copy link
Member


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@@ -38,7 +39,8 @@ class TriggerDagRunLink(BaseOperatorLink):
name = 'Triggered DAG'

def get_link(self, operator, dttm):
return f"/graph?dag_id={operator.trigger_dag_id}&root=&execution_date={quote(dttm.isoformat())}"
view = conf.get('webserver', 'dag_default_view').lower()
return f"/{view}?dag_id={operator.trigger_dag_id}&root=&execution_date={quote(dttm.isoformat())}"
Copy link
Member

Choose a reason for hiding this comment

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

Should we add base_url here?

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 afraid now it won't work properly if you run Airflow with base_url or [proxy-fix-prefix]https://airflow.readthedocs.io/en/latest/configurations-ref.html#proxy-fix-x-prefix] configured.

Copy link
Member

Choose a reason for hiding this comment

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

When Airflow is installed at http://localhost:8080/my-company/airflow, users will redirect to http://localhost:8080/graph instead of http://localhost:8080/my-company/airflow/graph.

Copy link
Member Author

Choose a reason for hiding this comment

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

When using base_url in the url and using breeze I'm redirected to localhost:8080 instead of 0.0.0.0:28080. So I think we don't have to use it it in url.

When using url_for then the link doesn't work at all

Choose a reason for hiding this comment

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

Looks like it remains an open issue for v2 when prefixing /base/ in base_url. Is there any issue open here for tracking?

airflow/utils/helpers.py Outdated Show resolved Hide resolved
@turbaszek

This comment has been minimized.

@turbaszek turbaszek force-pushed the use-default-view-in-dr-op-link branch from 99fd7c2 to 72daac8 Compare November 2, 2020 11:20
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 2, 2020
@github-actions
Copy link

github-actions bot commented Nov 2, 2020

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebased your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

@turbaszek turbaszek force-pushed the use-default-view-in-dr-op-link branch 2 times, most recently from 1a9c4af to a90b692 Compare November 4, 2020 09:19
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@turbaszek turbaszek force-pushed the use-default-view-in-dr-op-link branch from a953de0 to 0e3855e Compare November 8, 2020 17:36
@turbaszek
Copy link
Member Author

@kaxil @mik-laj happy to get another round of reviews 😄

@turbaszek turbaszek merged commit 289c9b5 into apache:master Nov 11, 2020
@turbaszek turbaszek deleted the use-default-view-in-dr-op-link branch November 11, 2020 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants