Skip to content

Conversation

@jedcunningham
Copy link
Member

We aren't going to land AIP-52 in time for 2.6, so put the authoring api behind a feature flag. I've chosen to put it in airflow.settings so users can set it in airflow_local_settings, or set it via env var.

@boring-cyborg boring-cyborg bot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers area:serialization labels Apr 6, 2023
@jedcunningham jedcunningham added the AIP-52 Automatic setup and teardown tasks label Apr 6, 2023
@potiuk
Copy link
Member

potiuk commented Apr 6, 2023

We should do the same with AIP-44 I guess.

What's the idea about running tests - should AIRFLOW_ENABLE_AIP_52 be still enabled in CI ? I'd say yes as we are going to continue developing it but then we should disable it in the 2.6 branch so that the tests are not run there? Is my thinking right? Should we add it then in in ci.yml at the top of the workflow?

@jedcunningham
Copy link
Member Author

I was thinking about the CI side of this this morning, and I could make an argument either way. I went for "off in main for now" as that'd match what non-AIP-52 contributors care about and would run locally, but I'm happy to enable it for main and disable it in the 2.6 branch though. Commit incoming.

@jedcunningham
Copy link
Member Author

Actually, I'll make that change after I get a successful run here, just to make sure I didn't miss any tests relying on it.

@potiuk
Copy link
Member

potiuk commented Apr 6, 2023

Actually, I'll make that change after I get a successful run here, just to make sure I didn't miss any tests relying on it.

Went the same route in my PR :) #30510 - will get it succeed and flip the flag then.

@potiuk
Copy link
Member

potiuk commented Apr 6, 2023

I was thinking about the CI side of this this morning, and I could make an argument either way.

My argument is that if we don't enable it in main, then those contributors who don't care might (accidentally) break any work done by those who work on the in-progress AIP, so having it in main is a safeguard not to break what's already in main.

@jedcunningham
Copy link
Member Author

Okay, this is ready for a final review now 👍

@potiuk
Copy link
Member

potiuk commented Apr 7, 2023

FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham

@potiuk
Copy link
Member

potiuk commented Apr 7, 2023

OK. I merged my change first - this one will need conflict resolution now on settings, but after rebase, it will also run a separate job with AIRFLOW_ENABLE_AIP_52: "false" on top of all the tests run with it set to "true"

@jedcunningham
Copy link
Member Author

Nice, good call @potiuk!

@jedcunningham jedcunningham force-pushed the put_setup_teardown_behind_flag branch from 3cf29b6 to b865a84 Compare April 7, 2023 15:31
@potiuk
Copy link
Member

potiuk commented Apr 7, 2023

Nice, good call @potiuk!

You might want to rebase AGAIN to account for just merged #30521 that actually makes a difference for those two jobs (the variables were not propagated to docker environment so they had no effect).

We aren't going to land AIP-52 in time for 2.6, so put the authoring api
behind a feature flag. I've chosen to put it in `airflow.settings` so
users can set it in `airflow_local_settings`, or set it via env var.
@jedcunningham jedcunningham force-pushed the put_setup_teardown_behind_flag branch from b865a84 to 316fcbd Compare April 7, 2023 15:52
@potiuk
Copy link
Member

potiuk commented Apr 7, 2023

Nice. I see skipped AIP-52 tests in the separate job

@potiuk
Copy link
Member

potiuk commented Apr 7, 2023

And they are not skipped in regular tests . All looks cool

@jedcunningham jedcunningham merged commit 0fe73c2 into apache:main Apr 7, 2023
@jedcunningham jedcunningham deleted the put_setup_teardown_behind_flag branch April 7, 2023 20:23
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-52 Automatic setup and teardown tasks area:providers area:serialization changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

6 participants