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

Impossible to get a value from env #79

Closed
nsitbon opened this issue Nov 27, 2019 · 7 comments · Fixed by #80
Closed

Impossible to get a value from env #79

nsitbon opened this issue Nov 27, 2019 · 7 comments · Fixed by #80

Comments

@nsitbon
Copy link

nsitbon commented Nov 27, 2019

I have the exact same issue as #30 when upgrading from v0.8.0 to v0.9.0.

@rogpeppe
Copy link
Contributor

Could you supply some code that reproduces the issue, please?
I tried the following code:

package main

import (
	"context"
	"fmt"

	"github.com/heetch/confita"
	"github.com/heetch/confita/backend/env"
	"github.com/heetch/confita/backend/flags"
)

type Config struct {
	Host string `config:"host,required"`
}

func main() {
	var cfg Config
	l := confita.NewLoader(env.NewBackend(), flags.NewBackend())
	err := l.Load(context.Background(), &cfg)
	if err != nil {
		fmt.Printf("error %v\n", err)
	} else {
		fmt.Printf("ok %q\n", cfg.Host)
	}
}

I ran it with both the environment variable $HOST set and the command line flag -host set, and it seemed to work OK both times. I cannot reproduce the issue.

@nsitbon
Copy link
Author

nsitbon commented Nov 28, 2019

Hi thank you for your prompt answer.
My bad the guilty tag seems to be v0.8.0:

package main

import (
	"context"
	"fmt"

	"github.com/heetch/confita"
	"github.com/heetch/confita/backend/env"
	"github.com/heetch/confita/backend/flags"
)

type Config struct {
    Port int `config:"port,required"`
	Host string `config:"host,required"`
}

func main() {
	var cfg Config
	l := confita.NewLoader(env.NewBackend(), flags.NewBackend())
	err := l.Load(context.Background(), &cfg)
	if err != nil {
		fmt.Printf("error %v\n", err)
	} else {
        fmt.Printf("ok %s:%d\n", cfg.Host, cfg.Port)
	}
}

with v0.7.0:

$ PORT=1234 ./proof --host=foo
ok foo:1234

with v0.8.0:

$ PORT=1234 ./proof --host=foo
error required key 'port' for field 'Port' not found

Let me know if you need more info.

Best regards

@rogpeppe
Copy link
Contributor

Is there some reason that you can't use v0.9.0 which has fixed this issue?

@nsitbon
Copy link
Author

nsitbon commented Nov 28, 2019

same issue with v0.9.0

@rogpeppe
Copy link
Contributor

same issue with v0.9.0

As I said above, I can't reproduce the issue with v0.9.0 (see the code I pasted). Does that code fail for you?

Could you provide some code that reproduces the issue for you under v0.9.0, please?

@nsitbon
Copy link
Author

nsitbon commented Nov 28, 2019

sure https://github.com/nsitbon/confita-bug-demo

~ » git clone [email protected]:nsitbon/confita-bug-demo.git
Clonage dans 'confita-bug-demo'...
remote: Enumerating objects: 35, done.
remote: Counting objects: 100% (35/35), done.
remote: Compressing objects: 100% (30/30), done.
remote: Total 35 (delta 1), reused 35 (delta 1), pack-reused 0
Réception d'objets: 100% (35/35), 34.30 Kio | 365.00 Kio/s, fait.
Résolution des deltas: 100% (1/1), fait.
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
~ » cd confita-bug-demo
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
~/confita-bug-demo(master) » ./build.sh
Sending build context to Docker daemon  222.7kB
Step 1/6 : FROM golang:1.13.4-alpine3.10 as builder
 ---> 3024b4e742b0
Step 2/6 : WORKDIR /project
 ---> Using cache
 ---> fff3c8b7fd62
Step 3/6 : COPY . .
 ---> 137a2c6d5672
Step 4/6 : RUN CGO_ENABLED=0 go build -mod vendor -ldflags "-s -w" -o demo-cmd .
 ---> Running in 98249e2bd067
Removing intermediate container 98249e2bd067
 ---> bb4d0fde6045
Step 5/6 : FROM alpine:3.10.3
 ---> 965ea09ff2eb
Step 6/6 : COPY --from=builder /project/demo-cmd  /
 ---> Using cache
 ---> 2e1637a9d6fe
Successfully built 2e1637a9d6fe
Successfully tagged confita-bug-demo:v0.9.0
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
~/confita-bug-demo(master) » ./launch.sh
error required key 'port' for field 'Port' not found

and it works as expected on the v0.7.0 branch https://github.com/nsitbon/confita-bug-demo/tree/v0.7.0

@rogpeppe
Copy link
Contributor

Thanks very much! I've found the bug (introduced in #67). I'll submit a fix soon.

rogpeppe added a commit that referenced this issue Nov 28, 2019
In #67, the `isFlagSet` logic was changed so that it always returned
true, so even unmentioned flags would override previously set values.

This PR fixes that issue (fixes #79) and also changes the flag tests
to be somewhat more flexible and test this specific issue.

I haven't made any wider fixes because the concept of using command-line
flags as a backend is fundamentally broken in a way that cannot be fixed
(see #63), so will probably be removed in a future major version release
of confita.
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 a pull request may close this issue.

2 participants