-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend luigi.contrib.ecs module to support feeding additional kwargs to ecs run_task #3083
Conversation
luigi/contrib/ecs.py
Outdated
if self.command: | ||
if 'containerOverrides' in overrides: | ||
new_commands = [] | ||
for command in self.command: | ||
for container_override in overrides['containerOverrides']: | ||
if container_override['name'] == command['name']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is some deep nesting and is a little hard to follow the logic. Is there a way this could be cleaned up or visually simplified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will take a look at it this afternoon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a new commit that hopefully makes it clearer, let me know what you think.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If closed, you may revisit when your time allows and reopen! Thank you for your contributions. |
Has this project given up on accepting community PRs? |
@greenlaw , to my knowledge this project is still "alive", although very quiet. While I have write access to the repo as a community maintainer, I am not an admin nor employee of Spotify. I cannot speak to Spotify's stance on the luigi project. And while I do have write access, my project context and time grows stale as I no longer utilize luigi in my day job. As such, I have limited time to review and provide feedback here. As folks submit PRs, I'm happy to take a look. However, I cannot be the sole person reviewing all submissions. Nor can I commit to an SLA on my responses. Please feel welcome to review and provide feedback to other community submissions as your time and energy allows! For your PR, I apologize for approving your PR so long ago without merging. I've updated the branch and will merge once the checks pass. |
@dlstadther Thank you for the response and merging. I certainly understand constraints on time and appreciate your efforts to maintain this project, apparently by yourself. I was more just generally curious on the state of Luigi, as development velocity does seem to have died down a bit. Perhaps people are focusing their attention on other workflow engines like Argo, Prefect, Airflow, etc. I know that even though we have had success with Luigi, my team has been stretching the limits of what it was intended to do, and will likely be moving away from it for future projects. |
Description
This updates the
luigi.contrib.ecs
module by providing a new overridablerun_task_kwargs
property that can be used to feed additional keyword arguments to theboto3
ECSrun_task
call.Motivation and Context
The existing module only allows users to provide dynamic container commands via
containerOverrides
, so is very limited to running simple ECS tasks. With these changes, it's now possible to change the launch type on the fly (e.g. EC2 -> FARGATE), give a task additional ephemeral storage, specify custom network configuration, etc.These changes play nicely with the old version of the module and don't break the previous API.
Have you tested this? If so, how?
Yes, I updated
test/contrib/ecs_test.py
and all tests pass.