Skip to content
This repository has been archived by the owner on Nov 27, 2023. It is now read-only.

Fix OS's environment variable inheritance #1963

Closed

Conversation

ulyssessouza
Copy link
Contributor

What I did
This is meant to be an intermediate a solution for OS inherited environment variables in https://github.com/joho/godotenv

This fix relies on a fork of https://github.com/joho/godotenv on my repository https://github.com/ulyssessouza/godotenv
Also in an alternative branch on compose-go to point to this fork and implement the changes needed in the file parsing.

Related issue
Resolves #1917

Signed-off-by: Ulysses Souza <ulyssessouza@gmail.com>
Copy link
Contributor

@mat007 mat007 left a comment

Choose a reason for hiding this comment

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

Makes sense, is there a way we can hand binaries to the OP in #1917 to have confirmation it addresses his issue?

@mat007
Copy link
Contributor

mat007 commented Aug 4, 2021

Also PR is still a draft.

@ulyssessouza ulyssessouza marked this pull request as ready for review August 4, 2021 18:17
@ulyssessouza
Copy link
Contributor Author

ulyssessouza commented Aug 5, 2021

@mat007 We don't have a proper way to hand a binary to contributor by now. What I can propose is to checkout and build it.
(The easy way would be using the release page as a pre-release, but it's not meant to serve this propose).
Obs: Just notice that the bump on compose-go is not to it's main branch so if it's bumped after that it will miss the fix

@mat007
Copy link
Contributor

mat007 commented Aug 5, 2021

Obs: Just notice that the bump on compose-go is not to it's main branch so if it's bumped after that it will miss the fix

Ah, we would need to use our fork of godotenv in compose-go, do we want a PR there first?

@ndeloof
Copy link
Collaborator

ndeloof commented Aug 5, 2021

replace don't apply on transitive dependencies, so this fix can't be applied on compose-go "only"

@mat007
Copy link
Contributor

mat007 commented Aug 5, 2021

replace don't apply on transitive dependencies, so this fix can't be applied on compose-go "only"

Yes, we need it in both compose-go and compose-cli, but in this PR, if I understand correctly, compose-cli uses a fork of compose-go. I don’t think we want that, do we?

@ulyssessouza
Copy link
Contributor Author

I just made a PR on compose-go to remove the replace and use the fork explicitly. Once it's merged in compose-go I will update this one to use it.

@mat007
Copy link
Contributor

mat007 commented Aug 6, 2021

But can’t we use a replace in both projects?

@mat007
Copy link
Contributor

mat007 commented Aug 6, 2021

Oh right, no we can’t see golang/go#30354
That makes sense, and it’s what @ndeloof was actually saying 😬

@@ -19,7 +19,7 @@ require (
github.com/awslabs/goformation/v4 v4.15.6
github.com/buger/goterm v1.0.0
github.com/cnabio/cnab-to-oci v0.3.1-beta1
github.com/compose-spec/compose-go v0.0.0-20210722130045-6e1e1c2b26de
github.com/compose-spec/compose-go v0.0.0-20210729053941-ef8509c7a5eb
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be updated now that compose-spec/compose-go#165 has been merged.

@ulyssessouza
Copy link
Contributor Author

Close that in favor of #2002

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker v2 Beta can't handle env file without "=" after variable name
3 participants