Re: [PR] Add MwaaServerlessDeleteWorkflowOperator [airflow]
o-nikolas merged PR #66891: URL: https://github.com/apache/airflow/pull/66891 -- 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]
Re: [PR] Add MwaaServerlessDeleteWorkflowOperator [airflow]
john-jac commented on code in PR #66891: URL: https://github.com/apache/airflow/pull/66891#discussion_r3244179213 ## providers/amazon/tests/unit/amazon/aws/operators/test_mwaa_serverless.py: ## @@ -220,6 +221,61 @@ def test_template_fields(self): validate_template_fields(self.operator) +class TestMwaaServerlessDeleteWorkflowOperator: +def setup_method(self): +self.operator = MwaaServerlessDeleteWorkflowOperator( +task_id="delete_workflow", +workflow_arn=WORKFLOW_ARN, +) + [email protected](AwsBaseHook, "conn", new_callable=mock.PropertyMock) +def test_execute(self, mock_conn): +mock_client = mock.MagicMock() +mock_client.delete_workflow.return_value = {"WorkflowArn": WORKFLOW_ARN} +mock_conn.return_value = mock_client + +result = self.operator.execute({}) + + mock_client.delete_workflow.assert_called_once_with(WorkflowArn=WORKFLOW_ARN) +assert result == WORKFLOW_ARN + [email protected](AwsBaseHook, "conn", new_callable=mock.PropertyMock) +def test_execute_with_version(self, mock_conn): +op = MwaaServerlessDeleteWorkflowOperator( +task_id="delete_workflow", +workflow_arn=WORKFLOW_ARN, +workflow_version="abc123def456abc123def456abc123de", +) +mock_client = mock.MagicMock() +mock_client.delete_workflow.return_value = { +"WorkflowArn": WORKFLOW_ARN, +"WorkflowVersion": "abc123def456abc123def456abc123de", +} +mock_conn.return_value = mock_client + +result = op.execute({}) Review Comment: Non-blocking observation: Most delete operators in AMPP return None since the resource no longer exists. Returning workflow_arn here is fine (useful for logging downstream), just noting the inconsistency with other delete operators like S3TablesDeleteTableBucketOperator, GlueCatalogDeleteDatabaseOperator, etc. which return None. -- 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]
Re: [PR] Add MwaaServerlessDeleteWorkflowOperator [airflow]
john-jac commented on code in PR #66891: URL: https://github.com/apache/airflow/pull/66891#discussion_r3244176148 ## providers/amazon/tests/system/amazon/aws/example_mwaa_serverless.py: ## @@ -159,7 +159,7 @@ def delete_workflow(workflow_arn: str): update_workflow, Review Comment: Per maintainer feedback on recent PRs, please add # TEST SETUP, # TEST BODY, and # TEST TEARDOWN comments in the chain() call for consistency. Example: > ```chain( > # TEST SETUP > test_context, > create_bucket, > upload_workflow_yaml, > workflow_arn, > # TEST BODY > start_workflow, > wait_for_run, > update_workflow, > start_workflow_2, > stop_workflow_run, > # TEST TEARDOWN > delete_workflow, > delete_bucket, > ) > ``` -- 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]
Re: [PR] Add MwaaServerlessDeleteWorkflowOperator [airflow]
john-jac commented on code in PR #66891: URL: https://github.com/apache/airflow/pull/66891#discussion_r3244170126 ## providers/amazon/docs/operators/mwaa_serverless.rst: ## @@ -83,6 +83,20 @@ To update an existing Amazon MWAA Serverless workflow, use :start-after: [START howto_operator_mwaa_serverless_update_workflow] :end-before: [END howto_operator_mwaa_serverless_update_workflow] +.. _howto/operator:MwaaServerlessDeleteWorkflowOperator: Review Comment: Minor: The "Delete a Workflow" section is placed between "Update" and "Stop". Consider moving it after "Stop a Workflow Run" to match the natural lifecycle order: Create → Start → Stop → Update → Delete. Delete is typically the final teardown action. -- 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]
Re: [PR] Add MwaaServerlessDeleteWorkflowOperator [airflow]
john-jac commented on code in PR #66891: URL: https://github.com/apache/airflow/pull/66891#discussion_r3244167840 ## providers/amazon/tests/system/amazon/aws/example_mwaa_serverless.py: ## @@ -36,9 +37,8 @@ from tests_common.test_utils.version_compat import AIRFLOW_V_3_0_PLUS if AIRFLOW_V_3_0_PLUS: -from airflow.sdk import TriggerRule, task +from airflow.sdk import TriggerRule Review Comment: Verify that no other @task functions remain in this file after removing the task import. If there are none, this is fine. If any remain (e.g., setup helpers), the import removal will break them. -- 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]
Re: [PR] Add MwaaServerlessDeleteWorkflowOperator [airflow]
john-jac commented on code in PR #66891: URL: https://github.com/apache/airflow/pull/66891#discussion_r3244164924 ## providers/amazon/tests/unit/amazon/aws/operators/test_mwaa_serverless.py: ## @@ -220,6 +221,61 @@ def test_template_fields(self): validate_template_fields(self.operator) +class TestMwaaServerlessDeleteWorkflowOperator: +def setup_method(self): +self.operator = MwaaServerlessDeleteWorkflowOperator( +task_id="delete_workflow", +workflow_arn=WORKFLOW_ARN, +) + [email protected](AwsBaseHook, "conn", new_callable=mock.PropertyMock) +def test_execute(self, mock_conn): +mock_client = mock.MagicMock() +mock_client.delete_workflow.return_value = {"WorkflowArn": WORKFLOW_ARN} +mock_conn.return_value = mock_client + +result = self.operator.execute({}) + + mock_client.delete_workflow.assert_called_once_with(WorkflowArn=WORKFLOW_ARN) +assert result == WORKFLOW_ARN + [email protected](AwsBaseHook, "conn", new_callable=mock.PropertyMock) +def test_execute_with_version(self, mock_conn): +op = MwaaServerlessDeleteWorkflowOperator( +task_id="delete_workflow", +workflow_arn=WORKFLOW_ARN, +workflow_version="abc123def456abc123def456abc123de", +) +mock_client = mock.MagicMock() +mock_client.delete_workflow.return_value = { +"WorkflowArn": WORKFLOW_ARN, +"WorkflowVersion": "abc123def456abc123def456abc123de", +} +mock_conn.return_value = mock_client + +result = op.execute({}) + +mock_client.delete_workflow.assert_called_once_with( +WorkflowArn=WORKFLOW_ARN, +WorkflowVersion="abc123def456abc123def456abc123de", +) +assert result == WORKFLOW_ARN + [email protected](AwsBaseHook, "conn", new_callable=mock.PropertyMock) +def test_execute_not_found(self, mock_conn): +mock_client = mock.MagicMock() +mock_client.delete_workflow.side_effect = ClientError( +{"Error": {"Code": "ResourceNotFoundException", "Message": "not found"}}, "DeleteWorkflow" Review Comment: nit: Strengthen the pytest.raises to match the specific error code: > with pytest.raises(ClientError, match="ResourceNotFoundException"): > self.operator.execute({}) > This ensures the test validates the right error path, not just any ClientError. -- 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]
