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

Go: berglas pkg/auto imports must panic on failure #18

Closed
ahmetb opened this issue Apr 26, 2019 · 4 comments · Fixed by #20
Closed

Go: berglas pkg/auto imports must panic on failure #18

ahmetb opened this issue Apr 26, 2019 · 4 comments · Fixed by #20

Comments

@ahmetb
Copy link

ahmetb commented Apr 26, 2019

I'm doing

import _ "github.com/GoogleCloudPlatform/berglas/pkg/auto"

in my Go app and it starts/boots just fine (on Kubernetes, or on Cloud Run), only later I find out it actually printed some log lines with errors about secrets initialization.

As a user I expect app to crash ASAP as there's no point of moving the app forward if a requirement cannot be initialized. It's certain that app will crash, and crashing early is a very conventional readiness/health check (recognized by runtimes like Kubernetes or Cloud Run).

image

@ahmetb ahmetb changed the title Go: berglas pkg/auto imports must panic Go: berglas pkg/auto imports must panic on failrue Apr 26, 2019
@ahmetb ahmetb changed the title Go: berglas pkg/auto imports must panic on failrue Go: berglas pkg/auto imports must panic on failure Apr 26, 2019
@ahmetb
Copy link
Author

ahmetb commented Apr 26, 2019

What's actually bad in my case is that it leaves ENV_VAR=berglas://foo/bar and this causes the string to be actually used, and therefore weird errors like "could not be connected to the database, wrong password" which could make users think "uh oh, did my password just change".

@sethvargo
Copy link
Member

Hey @ahmetb

This was an early design decision, the primary goal being portability between runtimes (e.g. the same code should work on GCP, locally, and on another cloud).

Adding such behavior greatly impedes the local development process, especially since Go doesn't have a good way to conditionally include imports across environments.

It's worth noting that berglas exec doesn't have this challenge, as it's expected to exit when a ref fails.

Curious your thoughts?

@ahmetb
Copy link
Author

ahmetb commented Apr 26, 2019

If I have an env var set to berglas:// it means I want it to be replaced for that, I can’t think of a scenario where silently failing is desired behavior. I can think of all the scenarios where failing ASAP makes a lot of sense.

@ahmetb
Copy link
Author

ahmetb commented Apr 26, 2019

Thanks for fixing this!

@lock lock bot locked as resolved and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants