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

feat: conforming envfile parsing to docker compose envfile #3763

Closed
wants to merge 4 commits into from

Conversation

rushib1
Copy link

@rushib1 rushib1 commented Jun 24, 2023

Summary

This pull request aims to support escape characters and support docker-compose spec (Doesn't support Parameter expansion)

Implementation details

I have imported the files from docker compose reporsitory and used those files to parse the envfiles

Testing

New tests cover the changes:
yes

Description for the changelog

Feature: Envfile conforming to docker-compose envfile (Not including parameter expansion)

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rushib1 rushib1 requested a review from a team as a code owner June 24, 2023 14:44
@rushib1
Copy link
Author

rushib1 commented Jun 24, 2023

This pr is made for this issue #3429

@Realmonia
Copy link
Contributor

Realmonia commented Jun 27, 2023

Thanks for the contribution!

Several comments

  • Can you not import the file from another repo directly, but add a go.mod dependency, as well as confirming https://github.com/aws/amazon-ecs-agent/blob/master/agent/THIRD_PARTY.md with the license information from the original repo is updated?
  • I am seeing multiple dotenv package for go are availabe, have you evaluated those as well? Can you add the evaluation process to PR summary?

@rushib1
Copy link
Author

rushib1 commented Jun 28, 2023

  1. The docker-compose has the official implementation for their spec and is not separated as a different package. Since I did not want to import the whole docker-compose package just for the functionality of dotenv, I have imported only the necessary files
  2. Since it is the official implementation, I assumed it to be the only option for the job and haven't explored other functionality

Let me know if its okay to import the total docker-compose package just for the functionality of dotenv implementation.

@Realmonia
Copy link
Contributor

Yep please import the whole package so we can have dependabot to help maintain it.

@rushib1
Copy link
Author

rushib1 commented Jun 28, 2023

Hi @Realmonia, whenever I try go get github.com/compose-spec/compose-go it keeps updating other dependencies like docker to v24, go-units to v0.5, logrus to v1.9.0 and testify to v1.8.4 any idea what I should do now?

@Realmonia
Copy link
Contributor

Realmonia commented Jun 28, 2023

Hmm interesting. It does not make sense to me as those packages are not part of the dependency in go.mod of the compose-go package. Can you try if handle override of these dependencies to previous version would work? If not, maybe you can try this godotenv dependency https://github.com/joho/godotenv

@rushib1
Copy link
Author

rushib1 commented Jun 29, 2023

Even without the dependency, If I try to run the commands go get or go mod download inside agent folder the dependecies are getting updated.

-       github.com/docker/docker v20.10.24+incompatible
+       github.com/docker/docker v24.0.2+incompatible
        github.com/docker/go-connections v0.4.0
-       github.com/docker/go-units v0.4.0
+       github.com/docker/go-units v0.5.0

Looks like it was an issue with ecs-agent which was fixed recently in a different pr

@Realmonia
Copy link
Contributor

Realmonia commented Jun 29, 2023

Oh yeah I forgot that. That makes sense.

Can you add an unit test to test the env file parsing behavior when it contains escape?

1. multiple cases of shell escaping
2. json sample unquoted test
3. support for values that span multiple lines
@rushib1
Copy link
Author

rushib1 commented Jun 29, 2023

Just for your information, these new changes will break the existing spec of task-def
Each line in an environment file must contain an environment variable in VARIABLE=VALUE format. Spaces or quotation marks are included as part of the values - source

Example exisitng envfile that is working currently but will produce a completely different output with the above changes:

key1="value1
key2=value2"

output produced by old code:
key1 value will be "value1
key2 value will be value2"

output produced by new parsing:
key1 value will be

value1
key2=value2

@Realmonia
Copy link
Contributor

Hey sorry after further discussion with the team, we decide to not make this change for now. As you pointed out this conflicts with the current documentation; we will need our product team to be involved in, and have proper customer wording before we actually make such change. However, it's possible that we will do this in the future, and we will reopen & merge PR then.

Thanks again for the contribution and sorry again for the confusion!

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.

4 participants