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

Conversation

@jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Mar 28, 2021

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

@jscheffl jscheffl 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

@github-actions github-actions bot commented May 13, 2021

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 label May 13, 2021
@ashb ashb removed the stale 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

@ashb ashb May 13, 2021

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

@jscheffl jscheffl May 13, 2021

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

@ashb ashb May 13, 2021

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 approved these changes May 13, 2021
@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 ashb commented May 13, 2021

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

@jscheffl jscheffl force-pushed the origin/bugfix/issue-15023 branch from c13e1ed to 666088e May 13, 2021
@jscheffl
Copy link
Contributor Author

@jscheffl jscheffl commented May 13, 2021

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

@jscheffl
Copy link
Contributor Author

@jscheffl jscheffl commented May 13, 2021

Ready for review (again)

@github-actions
Copy link

@github-actions github-actions bot commented May 13, 2021

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
Copy link

@github-actions github-actions bot commented May 13, 2021

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

@github-actions github-actions bot commented May 13, 2021

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 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

@github-actions github-actions bot commented May 13, 2021

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

@github-actions github-actions bot commented May 14, 2021

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

@github-actions github-actions bot commented May 14, 2021

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
25 of 27 checks passed
@boring-cyborg
Copy link

@boring-cyborg boring-cyborg bot commented Jun 22, 2021

Awesome work, congrats on your first merged pull request!

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

@jscheffl jscheffl commented Jun 22, 2021

@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.

@jscheffl jscheffl deleted the origin/bugfix/issue-15023 branch Jun 22, 2021
@ashb
Copy link
Member

@ashb 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 added a commit to Jorricks/airflow that referenced this issue Jun 24, 2021
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
potiuk added a commit to potiuk/airflow that referenced this issue 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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants