shahar1 commented on code in PR #66929:
URL: https://github.com/apache/airflow/pull/66929#discussion_r3324756615
##########
providers/google/src/airflow/providers/google/suite/operators/sheets.py:
##########
@@ -33,7 +34,9 @@ class GoogleSheetsCreateSpreadsheetOperator(BaseOperator):
:param spreadsheet: an instance of Spreadsheet
https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets#Spreadsheet
- :param gcp_conn_id: The connection ID to use when fetching connection info.
Review Comment:
Please restore this parameter, seems to be deleted by mistake
##########
providers/google/src/airflow/providers/google/suite/operators/sheets.py:
##########
@@ -33,7 +34,9 @@ class GoogleSheetsCreateSpreadsheetOperator(BaseOperator):
:param spreadsheet: an instance of Spreadsheet
https://developers.google.com/sheets/api/reference/rest/v4/spreadsheets#Spreadsheet
- :param gcp_conn_id: The connection ID to use when fetching connection info.
+ :param drive_id: Shared Drive ID where the spreadsheet should be created.
+ This is useful when using a service account, since service accounts
+ do not have personal Drive storage. (templated)
Review Comment:
```suggestion
:param drive_id: Shared Drive ID where the spreadsheet should be created.
This is useful when using a service account, since service accounts
do not have personal Drive storage. When drive_id is set, only
spreadsheet["properties"]["title"] is used; all other Sheets-specific
properties are silently ignored. (templated)```
It's important to note (both here and in the operator).
##########
providers/google/tests/system/google/cloud/gcs/example_gcs_to_sheets.py:
##########
@@ -88,7 +99,10 @@ def create_connection(connection_id: str):
create_connection_task = create_connection(connection_id=CONNECTION_ID)
create_spreadsheet = GoogleSheetsCreateSpreadsheetOperator(
- task_id="create_spreadsheet", spreadsheet=SPREADSHEET,
gcp_conn_id=CONNECTION_ID
+ task_id="create_spreadsheet",
+ spreadsheet=SPREADSHEET,
+ gcp_conn_id=CONNECTION_ID,
+ drive_id=GDRIVE_ID,
Review Comment:
Considering that until now we didn't use `drive_id`, I think that we need a
branch task and a flag (env. var.) to control it, so you wouldn't have to break
existing behavior when running this test locally (and you'll have the
flexbility to test both).
Something like the following:
```python
# Add at top with other env vars (around line 47)
USE_GDRIVE = os.environ.get("use_gdrive", "false").lower() == "true"
GDRIVE_SECRET_ID = "gdrive_shared_folder_id"
# Add imports
from airflow.providers.google.common.utils.get_secret import get_secret
from airflow.providers.google.suite.hooks.drive import GoogleDriveHook
# Inside DAG context, after create_connection task (around line 90)
@task
def branch_spreadsheet_creation() -> str:
"""Route to Sheets API or Drive API path based on USE_GDRIVE env var."""
return "create_spreadsheet_gdrive" if USE_GDRIVE else
"create_spreadsheet_sheets"
@task
def get_shared_drive_id() -> str:
"""Fetch shared drive ID from Secret Manager."""
return get_secret(secret_id=GDRIVE_SECRET_ID).strip()
branch_task = branch_spreadsheet_creation()
get_drive_id_task = get_shared_drive_id()
# Sheets API path (no secret required)
create_spreadsheet_sheets = GoogleSheetsCreateSpreadsheetOperator(
task_id="create_spreadsheet_sheets",
spreadsheet=SPREADSHEET,
gcp_conn_id=CONNECTION_ID,
)
# Drive API path (requires secret + env var)
create_spreadsheet_gdrive = GoogleSheetsCreateSpreadsheetOperator(
task_id="create_spreadsheet_gdrive",
spreadsheet=SPREADSHEET,
gcp_conn_id=CONNECTION_ID,
drive_id=get_drive_id_task,
)
# DAG routing
[create_bucket, create_connection_task] >> branch_task >>
[create_spreadsheet_sheets, create_spreadsheet_gdrive]
[create_spreadsheet_sheets, create_spreadsheet_gdrive] >>
upload_sheet_to_gcs >> ..
```
##########
providers/google/tests/system/google/cloud/gcs/example_sheets.py:
##########
@@ -68,15 +71,23 @@
start_date=datetime(2021, 1, 1),
catchup=False,
tags=["example", "sheets"],
+ render_template_as_native_obj=True,
Review Comment:
Could you please implement it without setting
`render_template_as_native_obj=True`? (even at the cost of an additional task,
if needed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
