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

Resolve environments case-insensitively on Windows #257

Merged
merged 2 commits into from
May 5, 2022

Conversation

ikedam
Copy link
Contributor

@ikedam ikedam commented May 3, 2022

Please have a look on docker/compose#9431 for details.

compose-go extracts environment variables to Environment map[string]string and reference that later with simply Environment[varname].
It works case-sensitively but environment variables should be treated case-insensitively on Windows.

This change handles environment variables case-insensitively on Windows, without changing data structures on compose-go.

This change also adds exposed utils.EnvResolver function. I plan to use that for additional changes in https://github.com/docker/compose/ , handling option args like --build-arg with build sub command and -e with run sub command.
(like this: https://github.com/ikedam/compose/pull/1/files)

@ikedam ikedam requested a review from ndeloof as a code owner May 3, 2022 03:53
@ndeloof
Copy link
Collaborator

ndeloof commented May 3, 2022

Thanks for looking into this!
For legal reasons, all commits need to be signed-off. Please amend your commit and force push your branch:
git commit --amend --signoff && git push --force ...

types/config.go Outdated
@@ -32,7 +33,7 @@ type ConfigDetails struct {

// LookupEnv provides a lookup function for environment variables
func (cd ConfigDetails) LookupEnv(key string) (string, bool) {
v, ok := cd.Environment[key]
v, ok := utils.EnvResolver(cd.Environment)(key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

as EnvResolver is not used in any other place, I'd prefer we keep things simple with EnvResolverWithCase logic being directly included here

if ok {
return v, ok
}
if loweredEnvironment == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to allocate a full lowercase map on each lookup, could just do

key := strings.ToLower(s)
for k, v := range environment {
   if key == strings.ToLower(k) {
      return v
   }
}

"gotest.tools/v3/assert"
)

func Test_EnvResolverWithCase(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@ikedam
Copy link
Contributor Author

ikedam commented May 4, 2022

Thanks for looking into this! For legal reasons, all commits need to be signed-off. Please amend your commit and force push your branch: git commit --amend --signoff && git push --force ...

I've added signed-off line as described in https://github.com/compose-spec/compose-go/blob/master/CONTRIBUTING.md :

PS C:\workspace\compose-go> git log -n 1
commit 90f9d92dab00e1272c050960baeaaa8d8b15aaad (HEAD -> feature/9431_CaseInsensitiveEnv, ikedam/feature/9431_CaseInsensitiveEnv)
Author: ikedam <[email protected]>
Date:   Tue May 3 11:58:23 2022 +0900

    Resolve environments case-insensitively on Windows

    Signed-off-by: IKEDA Yasuyuki <[email protected]>
PS C:\workspace\compose-go>

But something might be wrong as this is the first time I wrote signed-off line.

Should I use the real name also in author line? Or should I add GPG signature? (git commit -S)

@ikedam ikedam force-pushed the feature/9431_CaseInsensitiveEnv branch from 2f6d078 to 85892a2 Compare May 4, 2022 02:10
@ikedam
Copy link
Contributor Author

ikedam commented May 4, 2022

Oh, I got it. I have to pass DCO action. I'll try that.

@ikedam ikedam force-pushed the feature/9431_CaseInsensitiveEnv branch from 85892a2 to 4fcc599 Compare May 4, 2022 02:17
@ikedam ikedam requested a review from ndeloof May 4, 2022 02:18
@ikedam
Copy link
Contributor Author

ikedam commented May 4, 2022

Thanks for the review. I fixed problems you pointed.

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM

@glours glours merged commit 0e9b532 into compose-spec:master May 5, 2022
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.

3 participants