Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@shahar1@HammadASiddiqui@potiuk@eladkal
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} 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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@shahar1@HammadASiddiqui@potiuk@eladkal
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } 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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

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

Remove deprecated "delegate_to" from GCP operators and hooks - #30748

Merged
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to
Apr 20, 2023
Merged

Remove deprecated "delegate_to" from GCP operators and hooks#30748
potiuk merged 1 commit into
apache:mainfrom
shahar1:remove-delegate-to

Conversation

@shahar1

@shahar1shahar1 commented Apr 19, 2023

Copy link
Copy Markdown
Contributor

related: #9461, #29088

  • This PR removes the deprecated delegate_to parameter from GCP operators, hooks, sensors, transfers, and triggers, as well as from firebase hook.
  • It also removes the parameter from Microsoft Azure, Presto, Trino, and gsuite transfers that interact with Google Cloud.
  • The delegate_to param will still be available only in gsuite and marketing platform hooks, operators, sensors, and transfers that don't interact with Google Cloud (see reasoning in Unclear documentation for the delegate_to parameter #9461).

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborgboring-cyborgBot added provider:cncf-kubernetes Kubernetes (k8s) provider related issues area:providers provider:Apache provider:amazon AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Apr 19, 2023
@shahar1
shahar1force-pushed the remove-delegate-to branch from c2f547d to 8aac2f4CompareApril 19, 2023 18:53

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

@shahar1

Copy link
Copy Markdown
ContributorAuthor

Need also the following:

  1. Add to all relevant provider.yaml with new breaking change version (Except google as there it's already set)
  2. Add entry in changelog of each provider that explains the breaking change for the specific provider. This is simply an entry of "Removed delegate_to parameter from operators x, y, z"

Done

@shahar1
shahar1 requested review from eladkalApril 20, 2023 07:09
@shahar1
shahar1force-pushed the remove-delegate-to branch from 7c484e7 to ae5f970CompareApril 20, 2023 07:14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will be added automatically by the release manager during release.
What we want to describe via the PR is information about that the commit message itself does not cover.
For example: what operators involve? what action users needs to take.. basically additional information that may help user to bridge the breaking changes faster

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@potiukpotiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to think about it, but this has potential of breaking much more than just one provider, i "request for changes" for now until we discuss it. More comments are coming

@potiukpotiukApr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we cannot just remove the parameter here, because this will break an important property of the providers we have that they can be upgraded independently.

While I see why removing such deprecated parameter is needed at some point in time doing it this way has rather profound consequences.

For example if somene would like to keep old AWS provider but would like to upgrade a google provider to the new version, this will break silently for AWS provider (which will not be upgraded) - even if delegate_to parameter has NOT been used.

The problem is that old providers are already passing this parameter even if it has not been used. It will default to None and None will passed down.

So for exmple if you have old AWS provider and new Google provider, and you do not have delegate_to parameter in your operator, the gcs_to_s3 provider will call the GCSHook with "delegate_to" parameter set to None and this will crash.

This is far too invasive. Yes. It's ok to remove that feature, but it's not ok to break another provider's usage of this hook even if the feature has not been used.

Proposal:

I am ok with removing the delegate to parameter, but this change should be done differently.

  • In all Google hooks using delegate_to, remove the delegate_to parameter but add **kwargs . This will stop the Hooks from crashing even if delegate_to=None parameter has been passed to it from an older version of Amazon or other providers.

  • In those hooks convert the warning into error. Smth like:

if kwargs.get('delegate_to') is not None:
raise RuntimeError("The `delegate_to` parameter has been deprecated before and finally removed in this version of Google Provider. You MUST convert it to `impersonate_chain`).

I think this should be left here pretty much indefinitely.

@shahar1shahar1Apr 20, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, including tests for the if statements in all GCP hooks

@shahar1
shahar1force-pushed the remove-delegate-to branch from ae5f970 to 07e1a71CompareApril 20, 2023 14:46

@eladkaleladkal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk
potiuk merged commit fbc1382 into apache:mainApr 20, 2023
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of apache#30748 caused the apache#30579 to fail
as delegate_to parameter has been removed.
eladkal pushed a commit that referenced this pull request Apr 22, 2023
Two PRs crossed and the result of #30748 caused the #30579 to fail
as delegate_to parameter has been removed.
@HammadASiddiqui

Copy link
Copy Markdown

Hi!
Can someone please guide me on this discussion

googleapis/google-api-python-client#2228

austinletson added a commit to austinletson/airflow that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in apache#9461, the delegate_to param was
deprecated in apache#29088 (and removed in apache#30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
Taragolis pushed a commit that referenced this pull request Nov 18, 2023
Restore delegate_to param to GoogleDiscoveryApiHook to allow to
specification of a service account for domain-wide delegation when using
GoogleDiscoveryApiHook to access Google APIs which support domain-wide
delegation but do not have a dedicated hook (such as Workspaces Admin
API).
Note that based on the discussion in #9461, the delegate_to param was
deprecated in #29088 (and removed in #30748) from many hooks
including GoogleDiscoveryApiHook. However, the delegate_to param was
not removed from the docstring for the GoogleDiscoveryApiHook
constructor.
Update GCP connection docs to reflect delegate_to param in
GoogleDiscoveryApiHook usage only when using Google APIs that support
domain-wide delegation.
@shahar1
shahar1 deleted the remove-delegate-to branch June 12, 2024 13:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providersprovider:amazonAWS/Amazon - related issuesprovider:cncf-kubernetesKubernetes (k8s) provider related issuesprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@shahar1@HammadASiddiqui@potiuk@eladkal