You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/06/28 08:03:36 UTC
[GitHub] [airflow] pmalafosse opened a new pull request #16685: reattach_prev_task ECSOperator
pmalafosse opened a new pull request #16685:
URL: https://github.com/apache/airflow/pull/16685
FYI @darwinyip
This PR adds a parameter reattach_prev_task to ECSOperator.
**Before:**
Until now we could use 'reattach' which was reattaching a running ECS Task (if there was one running) of the same 'family' instead of creating a new one. The problem was that if we had workflows using the same ECS Task Definition in several tasks, it didn't know which one to reattach.
**Now:**
Now with reattach_prev_task instead, when we launch a new ECS task, it will store temporarily the ECS Task ARN in XCOM. If there is an issue during the run (typically connection problem between Airflow and ECS for long-running tasks):
- self._start_task will store the ECS task ARN in XCOM (in a 'fake' task_id equal to f"{self.task_id}_task_arn"
- in the next execution, it will check if this task ARN is still running and if so it will reattach it to the operator, otherwise it will create a new one
- when the operator runs succesfully it will delete the XCOM value
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-872571932
cc @subashcanapathy If anyone in your team can review this, it would be great as it affects AWS Hooks
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #16685: Allow attaching to previously launched task in ECSOperator
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #16685:
URL: https://github.com/apache/airflow/pull/16685
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #16685: Allow attaching to previously launched task in ECSOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r670027434
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -349,6 +349,111 @@ def test_reattach_successful(self, launch_type, tags, start_mock, check_mock, wa
check_mock.assert_called_once_with()
assert self.ecs.arn == 'arn:aws:ecs:us-east-1:012345678910:task/d8c67b3c-ac87-4ffe-a847-4785bc3a8b55'
+ @parameterized.expand(
+ [
+ ['EC2', None],
+ ['FARGATE', None],
+ ['EC2', {'testTagKey': 'testTagValue'}],
+ ['', {'testTagKey': 'testTagValue'}],
+ ]
+ )
+ @mock.patch.object(ECSOperator, "_xcom_del")
+ @mock.patch.object(
+ ECSOperator,
+ "xcom_pull",
+ return_value="arn:aws:ecs:us-east-1:012345678910:task/d8c67b3c-ac87-4ffe-a847-4785bc3a8b55",
+ )
+ @mock.patch.object(ECSOperator, '_wait_for_task_ended')
+ @mock.patch.object(ECSOperator, '_check_success_task')
+ @mock.patch.object(ECSOperator, '_start_task')
+ def test_reattach_successful(
Review comment:
There is already a test with the same name on L349
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-880191905
Can you rebase on latest main please @pmalafosse
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-872077735
I just made the change to 'reattach' instead @darwinyip
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] darwinyip commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
darwinyip commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-871873235
This flag doesn't seem like a good idea. I agree with the underlying issue this is trying to solve.
What about just updating `reattach` directly with this new logic?
A more abrupt approach would be forcing every ECS task to take the name `airflow + dag_id + task_id` and have `reattach` check on that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-874987534
Looks fine to me. I'm surprised there aren't existing helpers for xcom get/put.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] subashcanapathy commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
subashcanapathy commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-874266599
@o-nikolas @ferruzzi Tagging for review
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] subashcanapathy commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
subashcanapathy commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-874266599
@o-nikolas @ferruzzi Tagging for review
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a change in pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r664782154
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -316,16 +316,27 @@ def test_check_success_task_not_raises(self):
['', {'testTagKey': 'testTagValue'}],
]
)
+ @mock.patch.object(ECSOperator, '_xcom_del')
+ @mock.patch.object(
+ ECSOperator,
+ "xcom_pull",
+ return_value="arn:aws:ecs:us-east-1:012345678910:task/d8c67b3c-ac87-4ffe-a847-4785bc3a8b55",
Review comment:
Just to verify, this task ARN is generic/random, right?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-875834825
@o-nikolas thanks for spotting that! There were 2 lines not using the constants yet indeed. Fixed now
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-875418607
Thanks a lot for the review @ferruzzi @o-nikolas , I made the small changes you mentioned
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-872048609
> This flag doesn't seem like a good idea. I agree with the underlying issue this is trying to solve.
> What about just updating `reattach` directly with this new logic? The flag only has a simple use case in mind with no consideration for concurrency, and that was okay at that time.
>
> A more abrupt approach would be forcing every ECS task to take the name `airflow + dag_id + task_id` and have `reattach` check on that.
Yes that makes sense, I will update 'reattach' logic directly then, I was not sure if it had an other user case I hadn't thought about.
Unfortunately we can't define the ECS task_id from our side, it is a unique ID generated by Amazon (and if we could there would be problems with retries and clearing tasks anyway).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16685: Allow attaching to previously launched task in ECSOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-880312980
Awesome work, congrats on your first merged pull request!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a change in pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r664939787
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -264,6 +276,21 @@ def _start_task(self):
self.log.info('ECS Task started: %s', response)
self.arn = response['tasks'][0]['taskArn']
+ ecs_task_id = self.arn.split("/")[-1]
+ self.log.info(f"ECS task ID is: {ecs_task_id}")
+
+ if self.reattach:
+ # Save the task ARN in XCom to be able to reattach it if needed
+ self._xcom_set(context, key="ecs_task_arn", value=self.arn, task_id=f"{self.task_id}_task_arn")
+
+ def _xcom_set(self, context, key, value, task_id):
Review comment:
This just contains a single line and is only used once, do we need the helper? Or did you use this mostly to make patching in your testing easier (which is completely valid IMHO)?
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -264,6 +276,21 @@ def _start_task(self):
self.log.info('ECS Task started: %s', response)
self.arn = response['tasks'][0]['taskArn']
+ ecs_task_id = self.arn.split("/")[-1]
+ self.log.info(f"ECS task ID is: {ecs_task_id}")
+
+ if self.reattach:
+ # Save the task ARN in XCom to be able to reattach it if needed
+ self._xcom_set(context, key="ecs_task_arn", value=self.arn, task_id=f"{self.task_id}_task_arn")
Review comment:
Minor nit: the magic string `"ecs_task_arn"` is used in a couple places. Perhaps put this in a constant. You could also use a templated string constant for the task_id
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -274,15 +301,14 @@ def _try_reattach_task(self):
)
running_tasks = list_tasks_resp['taskArns']
- running_tasks_count = len(running_tasks)
- if running_tasks_count > 1:
- self.arn = running_tasks[0]
- self.log.warning('More than 1 ECS Task found. Reattaching to %s', self.arn)
- elif running_tasks_count == 1:
- self.arn = running_tasks[0]
- self.log.info('Reattaching task: %s', self.arn)
+ # Check if the ECS task previously launched is already running
+ previous_task_arn = self.xcom_pull(task_ids=f"{self.task_id}_task_arn", key="ecs_task_arn")
+ self.log.info(f"Previously launched task = {previous_task_arn}")
Review comment:
Nit: It probably adds noise to log this if we're not going to use it. I think the log line inside the if statement below is sufficient.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on a change in pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r665147274
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -264,6 +276,21 @@ def _start_task(self):
self.log.info('ECS Task started: %s', response)
self.arn = response['tasks'][0]['taskArn']
+ ecs_task_id = self.arn.split("/")[-1]
+ self.log.info(f"ECS task ID is: {ecs_task_id}")
+
+ if self.reattach:
+ # Save the task ARN in XCom to be able to reattach it if needed
+ self._xcom_set(context, key="ecs_task_arn", value=self.arn, task_id=f"{self.task_id}_task_arn")
+
+ def _xcom_set(self, context, key, value, task_id):
Review comment:
yes exactly I added it for the patching in the testing
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-869492174
FYI @feluelle @o-nikolas @markhopson as you were involved in other PRs for this ECSOperator
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] darwinyip edited a comment on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
darwinyip edited a comment on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-871873235
This flag doesn't seem like a good idea. I agree with the underlying issue this is trying to solve.
What about just updating `reattach` directly with this new logic? The flag only has a simple use case in mind with no consideration for concurrency, and that was okay at that time.
A more abrupt approach would be forcing every ECS task to take the name `airflow + dag_id + task_id` and have `reattach` check on that.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-869464960
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on a change in pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r665147061
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -274,15 +301,14 @@ def _try_reattach_task(self):
)
running_tasks = list_tasks_resp['taskArns']
- running_tasks_count = len(running_tasks)
- if running_tasks_count > 1:
- self.arn = running_tasks[0]
- self.log.warning('More than 1 ECS Task found. Reattaching to %s', self.arn)
- elif running_tasks_count == 1:
- self.arn = running_tasks[0]
- self.log.info('Reattaching task: %s', self.arn)
+ # Check if the ECS task previously launched is already running
+ previous_task_arn = self.xcom_pull(task_ids=f"{self.task_id}_task_arn", key="ecs_task_arn")
+ self.log.info(f"Previously launched task = {previous_task_arn}")
Review comment:
I will remove it, thanks!
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16685: Allow attaching to previously launched task in ECSOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-880242336
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-880217176
> done @kaxil it's the first time I do that so I'm not sure I did it right
> I followed those instructions FYI https://medium.com/@topspinj/how-to-git-rebase-into-a-forked-repo-c9f05e821c8a
You did it right :) -- let's wait for the CI to pass.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: Allow attaching to previously launched task in ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-880238325
Sorry there were some conflicts after the rebase that I hadn't fixed completely, fixed now (I ran pytest locally and it passed) @kaxil .
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on a change in pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r665609681
##########
File path: airflow/providers/amazon/aws/operators/ecs.py
##########
@@ -303,7 +310,6 @@ def _try_reattach_task(self):
# Check if the ECS task previously launched is already running
previous_task_arn = self.xcom_pull(task_ids=f"{self.task_id}_task_arn", key="ecs_task_arn")
Review comment:
This line should be updated to use your new constants?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] o-nikolas commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
o-nikolas commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-875829948
> Thanks a lot for the review @ferruzzi @o-nikolas , I made the small changes you mentioned
Thanks! I reviewed the latest commit, and it looks like there's one remaining line where you should be using the new constants. Let me know if I'm mistaken.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on pull request #16685: reattach_prev_task ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#issuecomment-880210381
done @kaxil it's the first time I do that so I'm not sure I did it right
I followed those instructions FYI https://medium.com/@topspinj/how-to-git-rebase-into-a-forked-repo-c9f05e821c8a
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] pmalafosse commented on a change in pull request #16685: Allow attaching to previously launched task in ECSOperator
Posted by GitBox <gi...@apache.org>.
pmalafosse commented on a change in pull request #16685:
URL: https://github.com/apache/airflow/pull/16685#discussion_r670031677
##########
File path: tests/providers/amazon/aws/operators/test_ecs.py
##########
@@ -349,6 +349,111 @@ def test_reattach_successful(self, launch_type, tags, start_mock, check_mock, wa
check_mock.assert_called_once_with()
assert self.ecs.arn == 'arn:aws:ecs:us-east-1:012345678910:task/d8c67b3c-ac87-4ffe-a847-4785bc3a8b55'
+ @parameterized.expand(
+ [
+ ['EC2', None],
+ ['FARGATE', None],
+ ['EC2', {'testTagKey': 'testTagValue'}],
+ ['', {'testTagKey': 'testTagValue'}],
+ ]
+ )
+ @mock.patch.object(ECSOperator, "_xcom_del")
+ @mock.patch.object(
+ ECSOperator,
+ "xcom_pull",
+ return_value="arn:aws:ecs:us-east-1:012345678910:task/d8c67b3c-ac87-4ffe-a847-4785bc3a8b55",
+ )
+ @mock.patch.object(ECSOperator, '_wait_for_task_ended')
+ @mock.patch.object(ECSOperator, '_check_success_task')
+ @mock.patch.object(ECSOperator, '_start_task')
+ def test_reattach_successful(
Review comment:
Thanks, I removed the first one as it was redundant with this new one, more complete
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org