Add support for fetcing private key from an environment variable

[JAtula]

Add support for fetching private key from an environment variable.

[bradleyfalzon]

This looks tentatively ok to me, but any reason you think this library should handle something from the environment when it could be easily handled from the application?

If we do decide to merge it (it’s a minor change) what’s your thoughts on adding some error handling in case the env variable is empty? Then I’d see a little more value as we’d be able to return an appropriate error without the calling app having to also handle that.

[JAtula]

Fetching from env became a need as I'm developing a little lambda API that uses the private key to authenticate and I don't want to package the key in app for sec reasons.

I'll add some logic to check if the env is empty 😎

[bradleyfalzon]

Sorry just on mobile, but “if !ok” is probably better.

I still don’t see exactly why this needs to be done within the library, your explanation was sound, storing these in environment is perfectly acceptable for many reasons, but I’d have thought the application itself should have been handling all this.

Having said that, we do handle opening from file for the user, so it seems ok to open from environment too. If you could make that minor change I’ll merge.

Thank you for the contribution and time you spent on this.

[JAtula]

Sorry just on mobile, but “if !ok” is probably better.

I still don’t see exactly why this needs to be done within the library, your explanation was sound, storing these in environment is perfectly acceptable for many reasons, but I’d have thought the application itself should have been handling all this.

Having said that, we do handle opening from file for the user, so it seems ok to open from environment too. If you could make that minor change I’ll merge.

Thank you for the contribution and time you spent on this.

ok != true is now !ok 😊

And thank you for making my life a lot easier with this lib!

[cruftyoldsysadmin]

I would +1 this. I'm in the same boat, running a GitHub webhook on AWS Lambda. My current hack is to read pass the key into an environment variable for the lambda, and then writing that out to a file before running NewKeyFromFile. A NewKeyFromVariable function would be great for this use case.

[koslib]

Any updates regarding this? I can provide hands-on help if needed to get this merged :) I'm currently in a similar situation with a hacky solution (read the file's content from a k8s secret, write it into a file, then shove it down to NewKeyFromFile) - would be awesome to support this out of the box in this lib (as I assume most people deploy with env/secrets, not files).

[wlynch]

FWIW, to support environment variables you can do something like:

itr, err := ghinstallation.New(http.DefaultTransport, appID, installationID, []byte(os.GetEnv("MY_ENV_VAR")))

This PR provides a wrapper around this, but you can still use env variables today without it 🤷

Similar for k8s secrets:

s, err := client.Secrets("namespace").Get(ctx, "my-secret", metav1.GetOptions{})
if err != nil {
  ...
}
itr, err := ghinstallation.New(http.DefaultTransport, appID, installationID, s.Data["my-key"])

(or if you're using file projection you can use NewKeyFromFile)

[koslib]

FWIW, to support environment variables you can do something like:

itr, err := ghinstallation.New(http.DefaultTransport, appID, installationID, []byte(os.GetEnv("MY_ENV_VAR")))

This PR provides a wrapper around this, but you can still use env variables today without it 🤷

Similar for k8s secrets:

s, err := client.Secrets("namespace").Get(ctx, "my-secret", metav1.GetOptions{})
if err != nil {
  ...
}
itr, err := ghinstallation.New(http.DefaultTransport, appID, installationID, s.Data["my-key"])

(or if you're using file projection you can use NewKeyFromFile)

To be honest, you are absolutely correct! For some reason I had missed this .New() method entirely and was stuck in the file-based one!

Thanks for pointing this out!