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

Ensure that dag_run conf is a dict #15057

Merged
merged 12 commits into from Jun 22, 2021
Merged

Ensure that dag_run conf is a dict #15057

merged 12 commits into from Jun 22, 2021

Conversation

jscheffl1
Copy link
Contributor

With this PR new DAG triggers are validated that the submitted conf (=dag_run_conf) is a dictionary. Previously submissions were possible and the logic just validated if the parameter is a valid JSON. Nevertheless currently some business logic fails in Airflow if the con value is not a dictionary object.

Explicitly I added the validation NOT to the data model as instances with migrated legacy data (in Airflow 1.x non dictionary cases were supported) are not failing in loading data via ORM.

The PR mainly adds validation for Legacy Experimental API and WWW UI.
It adds pytest for Legacy Experimental API, "new" API and WWW UI.

Update Pylint and flynt

closes: #15023
related: #15023

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:providers area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues provider:Apache labels Mar 28, 2021
@jscheffl1 jscheffl1 changed the title Origin/bugfix/issue 15023 Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict Mar 28, 2021
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label May 13, 2021
@ashb ashb removed the stale Stale PRs per the .github/workflows/stale.yml policy file label May 13, 2021
@@ -426,7 +426,7 @@ def load_file(
if create or recreate:
if field_dict is None:
raise ValueError("Must provide a field dict when creating a table")
fields = ",\n ".join(['`{k}` {v}'.format(k=k.strip('`'), v=v) for k, v in field_dict.items()])
fields = ",\n ".join([f"`{k.strip('`')}` {v}" for k, v in field_dict.items()])
Copy link
Member

Choose a reason for hiding this comment

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

This change looks unrelated -- is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @ashb Yes, the changes in hive.py were unrelated but the test scripts failed on this preventing me to open the PR. I needed to fix there... don't know why :-(

Copy link
Member

Choose a reason for hiding this comment

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

It should be ignored if you don't otherwise change the file.

@ashb ashb changed the title Origin/bugfix/issue #15023 / Add validation for dag_run conf to be a dict Ensure that dag_run conf is a dict May 13, 2021
@ashb
Copy link
Member

ashb commented May 13, 2021

@jscheffl Could you rebase this change please? (We split tests_views.py in to multiple files).

@jscheffl1
Copy link
Contributor Author

@ashb uups, something went bad in the rebase, will need one further attempt to integrate on rebase...

@jscheffl1
Copy link
Contributor Author

Ready for review (again)

@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label May 13, 2021
@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$,^Provider 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$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb
Copy link
Member

ashb commented May 13, 2021

tests/www/views/test_views_trigger_dag.py:87:30: F821 undefined name 'DR'
tests/www/views/test_views_trigger_dag.py:87:41: F821 undefined name 'DR'

@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$,^Provider 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$,^Provider 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$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb ashb merged commit 01c9818 into apache:main Jun 22, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 22, 2021

Awesome work, congrats on your first merged pull request!

ashb pushed a commit that referenced this pull request Jun 22, 2021
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
(cherry picked from commit 01c9818)
@jscheffl1
Copy link
Contributor Author

@ashb Thanks for finalizing the merge! Looking forward for the next PR - I already have an idea what to contribute next... and hope I'll find some weekend time soon.

@jscheffl1 jscheffl1 deleted the origin/bugfix/issue-15023 branch June 22, 2021 20:45
@ashb
Copy link
Member

ashb commented Jun 23, 2021

@jscheffl This PR will be in 2.1.1 too -- I merged it just before the freeze for release prep

Jorricks pushed a commit to Jorricks/airflow that referenced this pull request Jun 24, 2021
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
potiuk added a commit to potiuk/airflow that referenced this pull request Jul 5, 2021
There was a bad cherry-pick when merging apache#15057 that has not
been caught because of quarantined test.

Fixes: apache#16810
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API area:providers area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DAG task execution and API fails if dag_run.conf is provided with an array or string (instead of dict)
3 participants