Skip to content
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

Use EnvironmentFile instead of Environment #1762

Merged
merged 5 commits into from
Feb 3, 2023

Conversation

rariyama
Copy link
Contributor

Overview

fix #1749

I made a change that environment file is used when passing the values of environment to the container instead of environment, that is the way to add them to the container one by one.
This change will be effective to avoid failing an execution of ECS Run Task because a maximum size of characters of ECS container overrides is 8192 and this limit includes environments defined by the current implementation.
it will be helpful especially when executing sh>: operator. In addition to variables defined in _env, variables defined in _export are also passed to the container unlike py>: and rb>: so the size of environment tends to be larger as the development progresses.

The change points

@rariyama
Copy link
Contributor Author

rariyama commented Aug 2, 2022

I added the parameter for choosing the existing way in this commit 27d1ae3.
Users can select the either way for passing environments to a container by setting it. The default value is false, so there will be no impact for existing users.
If some users want to use environment file on most of tasks in workflow, they can use it easily by setting the parameter on the root task and override it on child ones as necessary.

@yoyama
Copy link
Contributor

yoyama commented Dec 23, 2022

Sorry for very late reply.
I don't check it in detail, but I have a few concerns.

  • I think providing an option to use file to pass environment variables is good and default is false is also good.
    But I think it is better to be configured by server properties because it is internal behavior.
  • Your change increases the code line of the method. I hope refactoring on your change.

fix document

extract process about uploading environmentFile to S3 as a method
@rariyama rariyama force-pushed the feature/use_environment_file branch from af9aa14 to fe6f8bf Compare January 6, 2023 18:10
@rariyama
Copy link
Contributor Author

rariyama commented Jan 6, 2023

@yoyama
Thanks for your reply.

I think providing an option to use file to pass environment variables is good and default is false is also good.
But I think it is better to be configured by server properties because it is internal behavior.

It makes sense to me, so I modified my implementation with this commit fe6f8bf. I left setEcsContainerOverrideEnvironment and added a process about using environmentFiles to it.

Your change increases the code line of the method. I hope refactoring on your change.

I also dealt with this concern with the commit above. I extracted a process about uploading environmentFile to S3 as another method from setEcsContainerOverrideEnvironment.

If there are any other concerns, please let me know about those.

Copy link
Member

@szyn szyn left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply.
I agree with adding ecs.<name>.use_environment_file to the system property. Actually, this property can also be set from the task config, but since the default is false and does not affect the behavior, it should be fine.

I have left some minor comments, I would appreciate it if you take a look at them when you get a chance.

fix storage type with s3

remove unnecessary variable
@rariyama
Copy link
Contributor Author

rariyama commented Feb 3, 2023

@szyn
Thanks for your review. I fixed the problems you commented so please confirm it when you are available.

Copy link
Member

@szyn szyn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you for your contribution.

@szyn szyn merged commit 9bd7cd6 into treasure-data:master Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InvalidParameterException occurred only when executing sh operator on ECSCommandExecutor.
3 participants