Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy
, '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

Dataplex operators - #20377

Merged
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators
Mar 14, 2022
Merged

Dataplex operators#20377
turbaszek merged 14 commits into
apache:mainfrom
lwyszomi:dataplex-operators

Conversation

@wojsamjan

Copy link
Copy Markdown

Add support for Google Dataplex. Includes operators, sensors, hooks, example dags, tests and docs.

Authored-by: Wojciech Januszek januszek@google.com


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

NIT: We prefer to use "%" format for logs, otherwise the string interpolation will be executed indepnendently of the logging level set (yep. I know INFO is default, but it can be changed to ERROR and then it is unnecessary to interpolate it)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ok, noted

Comment threadairflow/providers/google/cloud/hooks/dataplex.py Outdated

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.

Would you mind adding catchup=False? This has been added to all example DAGs to ward off any unexpected DagRuns for users if they copy this DAG for their use and modify start_date or schedule_interval without knowing about the catchup functionality.

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.

Could you add parameter/type info in the docstring for this hook? It would be great to see these in the Airflow API documentation which is generated by the docstring.

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.

Is this something that users should be able to configure in an Airflow Connection and passed to the hook or is the idea that this key must be configured within the environment itself?

If the latter, should there be a validation here to check that the api_key was provided rather than have the "INVALID API KEY" default value? Or is this default value checked somewhere downstream?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In Airflow Connection I set standard Keyfile Path - pointing to: /files/airflow-breeze-config/keys/<KEY_FILE_NAME>.json On the other hand we have to set an API Key in Credentials on GCP side to connect with discovery API - otherwise we can not perform operations.
I have not seen an option to set this value inside the Airflow Connection - so I used an environment variable. I am open to any suggestions about where it should be stored and any other improvements.

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.

Now that 2.2.3 has been released, typing for context can be:

fromtypingimportTYPE_CHECKINGifTYPE_CHECKING:
fromairflow.utils.contextimportContext
...
defexecute(self, context: "Context") ->dict:
...

This can be applied to all of the operators too.

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.

TIL 🚀 Thanks @josh-fell

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.

Yeah. I applied it globally during the Xmas break. Just wonder. Maybe we should add a pre-commit checking if the "old ways" are still used. WDYT @turbaszek@josh-fell ?

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.

+1 Definitely worth automating.

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.

Same comment here about context typing.

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.

Is that necessary?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, API Key is needed to perform operations on dataplex

@turbaszekturbaszekJan 8, 2022

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.

The authentication should be provided via dedicated connection. And as far as I remember GoogleBaseHook already provides all authentication methods supported by Google. If this is something only Dataplex specific we should introduce a new connection type. In this way users will have full control over the credentials. See for example google ads:

This hook requires two connections:
- gcp_conn_id - provides service account details (like any other GCP connection)
- google_ads_conn_id - which contains information from Google Ads config.yaml file
in the ``extras``. Example of the ``extras``:
.. code-block:: json
{
"google_ads_client": {
"developer_token": "{{ INSERT_TOKEN }}",
"path_to_private_key_file": null,
"delegated_account": "{{ INSERT_DELEGATED_ACCOUNT }}"
}
}
The ``path_to_private_key_file`` is resolved by the hook using credentials from gcp_conn_id.
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on how Google Ads authentication flow works take a look at:
https://developers.google.com/google-ads/api/docs/client-libs/python/oauth-service
.. seealso::
For more information on the Google Ads API, take a look at the API docs:
https://developers.google.com/google-ads/api/docs/start

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed within the team, for now I am going to remove API_KEY - it was needed for development purposes. Once the Dataplex API will be publicly available it will not be needed any more. I will commit changes and then draft this PR.

@wojsamjan
wojsamjan marked this pull request as draft January 12, 2022 14:28
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actionsgithub-actionsBot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Feb 27, 2022
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 6 times, most recently from 6bf36e6 to 904f091CompareMarch 9, 2022 09:39
@eladkaleladkal removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 9, 2022
@wojsamjan
wojsamjan marked this pull request as ready for review March 10, 2022 09:19
@wojsamjan
wojsamjanforce-pushed the dataplex-operators branch 2 times, most recently from 4eda728 to d93fbb5CompareMarch 10, 2022 10:29
@wojsamjan

Copy link
Copy Markdown
Author

@mik-laj@vikramkoka guys could you do a review, would be great. Thank you

@turbaszekturbaszek 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.

Looks good to me 👌

@github-actionsgithub-actionsBot added the full tests needed We need to run full set of tests for this PR to merge label Mar 11, 2022
@github-actions

Copy link
Copy Markdown
Contributor

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk

Copy link
Copy Markdown
Member

It needs at least rebase and checking if the errors were accidental.

@turbaszek
turbaszek merged commit 87c1246 into apache:mainMar 14, 2022
@ephraimbuddyephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Apr 11, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providerschangelog:skipChanges that should be skipped from the changelog (CI, tests, etc..)full tests neededWe need to run full set of tests for this PR to mergekind:documentationprovider:googleGoogle (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@wojsamjan@potiuk@turbaszek@eladkal@josh-fell@ephraimbuddy