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

Fixed DAG triggering with parameters #16815

Closed
wants to merge 1 commit into from

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 5, 2021

There was a bad cherry-pick when merging #15057 that has not
been caught because of quarantined test.

Fixes: #16810


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

There was a bad cherry-pick when merging apache#15057 that has not
been caught because of quarantined test.

Fixes: apache#16810
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jul 5, 2021
@potiuk potiuk requested review from jhtimmins and kaxil July 5, 2021 13:07
@kaxil
Copy link
Member

kaxil commented Jul 5, 2021

Looks like we already had a fix: #16648 in main .

@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 Jul 5, 2021
@github-actions
Copy link

github-actions bot commented Jul 5, 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 main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2021

Ok. Let me merge this one and we can see how the de-quarantining PR is doing.

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2021

I think we need to come back to the idea of keeping status of quarantined tests in issues (and some way of nagging the committers to fix them).

I have high hopes for the new GitHub issues (Private Beta) to be able to it much in much simpler and more manageable way (working with GitHub PM for GitHub issues to enable it for apache organisation - hopefully next week).

The new GitHub issues have better ways to organise them, split into sub-issues and better API for automation of any tasks around those (apparently). https://github.com/features/issues/

@kaxil
Copy link
Member

kaxil commented Jul 5, 2021

Meanwhile I have cherry-picked the original commit to both v2-1-stable and v2-1-test

And separately created #16819 to require approvals for mering to v2-1-stable

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2021

The test is apparently failing in main even AFTER the #16648 fix in main - so I think it was not flaky - it was marked as quarantined because it just did not work :). Surprisingly it worked for me when I fixed it in v2_1_stable PR. Let's see how this one will do - maybe it is a side-effect of some other tests.

The refactors on test_views do not make it easy to track the origin (I cannot see an issue created for that - I try to always create an issue when marking something as quarantined) but I tracked down when it was marked quarantined - Oct 11 2020

5bc5994

-    @pytest.mark.xfail(condition=using_mysql, reason="This test might be flaky on mysql")
+    @pytest.mark.quarantined
     def test_trigger_dag_conf(self):

Now. Let's see if I can track how long time ago we expected it to fail :)

@kaxil
Copy link
Member

kaxil commented Jul 5, 2021

Yeah must be a side-effect, as the test passes for me locally

@potiuk
Copy link
Member Author

potiuk commented Jul 5, 2021

Feb 6 2020: f0c31c5

OK: Seems the test was Never expected to work. It was added as "xfail" from the beginning :)

  • @pytest.mark.xfail(condition=True, reason="This test might be flaky on mysql")
  • def test_trigger_dag_conf(self):

@potiuk
Copy link
Member Author

potiuk commented Jul 6, 2021

Cosing this as already cherry-picked by @kaxil. Looking at quarantine removal in #16818

@potiuk potiuk closed this Jul 6, 2021
@potiuk potiuk deleted the fix-trigger-dag-run-conf branch July 29, 2022 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

3 participants