on_warning_callback does not work with astronomer-cosmos==1.1.0

[navedgaras]

Hi, just wanted to share that when using astronomer-cosmos==1.1.0 on_warning_callback functionality no longer works, it works on 1.0.5, and I believe this commit is at fault:
c7a203a

[marco9663]

It is also not working on version 1.1.1.

[tatiana]

Hi @edgga and @marco9663 , thanks for sharing this bug!
This feature didn't have any functional tests - would you be willing to help write a test case and fix the issue?
We can aim to have this fixed by 1.1.2 or 1.2!

[tatiana]

Hey @edgga and @marco9663, it seems we made three PRs to fix this issue: #556, #557, and #558 - if I had realised you were working on this, I wouldn't have spent any time on it 😂

The very good news is that the issue will undoubtedly be solved for the 1.1.2 release! 🎉

The PRs look very similar - the fix seems identical in the three of them, and what differs are the tests. Check the three PRs and vote (comment in this thread) on which test strategy you prefer and why.

I suggest we merge the PR with the preferred testing strategy, WDYT? Also happy to have the three of us as co-authors if you agree.

[marco9663]

Hey @edgga and @tatiana

I am happy with voting and co-author.

Just that I think #556 missed the condition of whether to run the on_warning_callback, i.e. it will always run on_warning_callback if provided.

[navedgaras]

Hi, @marco9663 and @tatiana ,

I have removed the _should_run_tests() method in my PR because the method name does not reflect what it does and also, because of a recent commit, test tasks should no longer be created for dbt models that do not have any tests (d70e6d7) so the if condition shouldn't be necessary.

As for tests, I think @tatiana did the best job out of all of us when testing the on_warning_callback functionality.

[tatiana]

Thanks a lot, @marco9663 and @edgga !
Nice catch, @edgga , I added that to #558.
Once the work is merged, we'll make the 1.1.2 release

[marco9663]

Nice catch @edgga !
I also prefer testing in #588.

[tatiana]

The fix was released as part of astronomer-cosmos 1.1.2. Thanks a lot to @marco9663 and @edgga for fixing the issue!

• https://github.com/astronomer/astronomer-cosmos/releases/tag/astronomer-cosmos-v1.1.2
• https://pypi.org/project/astronomer-cosmos/1.1.2/

There was a minor glitch while validating the release: #560, so I had to make some minor changes to the code we agreed - we should be able to "unrevert" #561 before the 1.2 release 🤞