Put AIP-52 setup/teardown tasks behind feature flag - #30509

Merged
jedcunningham merged 1 commit into
apache:mainfrom
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged

Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom
astronomer:put_setup_teardown_behind_flag

Conversation

@jedcunningham

@jedcunninghamjedcunningham commented Apr 6, 2023

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

@jedcunninghamjedcunningham added the AIP-52 Automatic setup and teardown tasks label Apr 6, 2023
norm
norm approved these changes Apr 6, 2023
View reviewed changes
@potiuk

potiuk commented Apr 6, 2023

Copy link
Copy Markdown
Member

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?

potiuk
potiuk reviewed Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py Outdated
@jedcunningham

jedcunningham commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

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

jedcunningham commented Apr 6, 2023

Copy link
Copy Markdown
MemberAuthor

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

potiuk commented Apr 6, 2023

Copy link
Copy Markdown
Member

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

potiuk commented Apr 6, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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.

@potiukpotiuk mentioned this pull request Apr 6, 2023
Merged
@jedcunningham

jedcunningham commented Apr 7, 2023

Copy link
Copy Markdown
MemberAuthor

Okay, this is ready for a final review now 👍

uranusjr
uranusjr reviewed Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py Outdated
@potiuk

potiuk commented Apr 7, 2023

Copy link
Copy Markdown
Member

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

@potiuk

potiuk commented Apr 7, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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"

@potiukpotiuk mentioned this pull request Apr 7, 2023
Merged
@jedcunningham

jedcunningham commented Apr 7, 2023

Copy link
Copy Markdown
MemberAuthor

Nice, good call @potiuk!

@jedcunningham
jedcunninghamforce-pushed the put_setup_teardown_behind_flag branch from 3cf29b6 to b865a84CompareApril 7, 2023 15:31
@potiuk

potiuk commented Apr 7, 2023

Copy link
Copy Markdown
Member

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

@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed the put_setup_teardown_behind_flag branch from b865a84 to 316fcbdCompareApril 7, 2023 15:52
@potiuk

potiuk commented Apr 7, 2023

Copy link
Copy Markdown
Member

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

@potiuk

potiuk commented Apr 7, 2023

Copy link
Copy Markdown
Member

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

@jedcunningham
jedcunningham merged commit 0fe73c2 into apache:mainApr 7, 2023
@jedcunningham
jedcunningham deleted the put_setup_teardown_behind_flag branch April 7, 2023 20:23
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2023
@eladkaleladkal mentioned this pull request May 4, 2023
Closed
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil
+1 more reviewer
@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Put AIP-52 setup/teardown tasks behind feature flag - #30509
Merged
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Apr 7, 2023
Merged
Put AIP-52 setup/teardown tasks behind feature flag#30509
jedcunningham merged 1 commit into
apache:mainfrom 
astronomer:put_setup_teardown_behind_flag
Conversation
@jedcunningham
@jedcunninghamjedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

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.
@jedcunninghamjedcunningham
added
the 
AIP-52
Automatic setup and teardown tasks
label
Apr 6, 2023
norm
norm
approved these changes
Apr 6, 2023
View reviewed changes
@potiuk
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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?
potiuk
potiuk
reviewed
Apr 6, 2023
View reviewed changes
Comment threadairflow/settings.py
Outdated

@jedcunningham
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
jedcunningham
commented
Apr 6, 2023

Copy link


Copy Markdown

MemberAuthor
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
potiuk
commented
Apr 6, 2023

Copy link


Copy Markdown

Member
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
potiuk
commented
Apr 6, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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.
@potiukpotiuk
mentioned this pull request
Apr 6, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Okay, this is ready for a final review now 👍
uranusjr
uranusjr
reviewed
Apr 7, 2023
View reviewed changes
Comment threadairflow/example_dags/example_setup_teardown_taskflow.py
Outdated

@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
FYI: with https://github.com/apache/airflow/pull/30510/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR965 we can both - have cake and eat it too @jedcunningham
@potiuk
potiuk
commented
Apr 7, 2023•

edited

Loading

Copy link


Copy Markdown

Member
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"
@potiukpotiuk
mentioned this pull request
Apr 7, 2023
 Merged

@jedcunningham
jedcunningham
commented
Apr 7, 2023

Copy link


Copy Markdown

MemberAuthor
Nice, good call @potiuk!
@jedcunningham
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
3cf29b6 to
b865a84CompareApril 7, 2023 15:31
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
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).
@jedcunningham
Put AIP-52 setup/teardown tasks behind feature flag
316fcbd
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
jedcunninghamforce-pushed
the
put_setup_teardown_behind_flag
branch
from
b865a84 to
316fcbdCompareApril 7, 2023 15:52
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
Nice. I see skipped AIP-52 tests in the separate job
@potiuk
potiuk
commented
Apr 7, 2023

Copy link


Copy Markdown

Member
And they are not skipped in regular tests . All looks cool
@jedcunningham
jedcunningham
merged commit 0fe73c2
into
apache:mainApr 7, 2023
@jedcunningham
jedcunningham
deleted the

put_setup_teardown_behind_flag

branch
April 7, 2023 20:23
@ephraimbuddyephraimbuddy
added
the 
changelog:skip
Changes that should be skipped from the changelog (CI, tests, etc..)
label
Apr 11, 2023
@eladkaleladkal
mentioned this pull request
May 4, 2023

Closed

2 tasks
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@potiukpotiukpotiuk approved these changes
@uranusjruranusjruranusjr left review comments
@ephraimbuddyephraimbuddyephraimbuddy approved these changes
@vincbeckvincbeckvincbeck approved these changes
@dstandishdstandishAwaiting requested review from dstandish
@ashbashbAwaiting requested review from ashbashb is a code owner
@kaxilkaxilAwaiting requested review from kaxil

+1 more reviewer

@normnormnorm approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

AIP-52Automatic setup and teardown tasksarea:providersarea:serializationchangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)provider:cncf-kubernetesKubernetes (k8s) provider related issues

Projects

No open projects
AIP-52 - Automatic setup and teardown tasks

Status: Done


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

6 participants

@jedcunningham@potiuk@norm@uranusjr@ephraimbuddy@vincbeck