Skip to content
This repository has been archived by the owner on Oct 6, 2022. It is now read-only.

feat: expose environment credentials provider #52

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MIJOTHY
Copy link

@MIJOTHY MIJOTHY commented Feb 22, 2022

Hopefully speaks for itself. I'm sure I've missed something though.

Edit:
Relates to
#53

@borkdude
Copy link
Contributor

Hopefully speaks for itself

It does not. Please describe the problem. Also provide a test when submitting a PR, since pod functionality always goes over the wire, it's not obvious that this change will work. Thanks!

@MIJOTHY
Copy link
Author

MIJOTHY commented Feb 22, 2022

Hopefully speaks for itself

It does not. Please describe the problem. Also provide a test when submitting a PR, since pod functionality always goes over the wire, it's not obvious that this change will work. Thanks!

Ah apologies: I see that all the other providers are exposed, so I assumed it was not a controversial change to also expose this one. I didn't see anything on why this specific provider is not exposed, but all the others are, so I figured it was an accidental omission. I would like to make use of this provider, rather than the others.

I'll look at adding some tests.

@borkdude
Copy link
Contributor

As I'm not working on this pod daily, some context in the form of an issue would be great.
Perhaps @jeroenvandijk also has some feedback on this.

@borkdude
Copy link
Contributor

@MIJOTHY Have you tested this PR locally? You can download the pod from one of the builds.

@MIJOTHY
Copy link
Author

MIJOTHY commented Feb 22, 2022

@MIJOTHY Have you tested this PR locally? You can download the pod from one of the builds.

I'm gonna turn this back to draft for now, while I push tests and test locally.

@MIJOTHY MIJOTHY marked this pull request as draft February 22, 2022 13:18
@jeroenvandijk
Copy link
Collaborator

@borkdude I haven't been working with this pod recently, but I can give some feedback by looking at the code.

@MIJOTHY The pod already exposes the (credentials/default-credentials-provider) by default and this falls back on environment variables. Can you explain why you want to use it directly? I have the feeling I'm missing something.

I don't know why I added it to the backend of the pod. I guess for completeness, but I forgot to properly test it. The implementation is not correct as it relies on the environment of the backend of the pod which could be potentially be different (e.g. start the pod in a different shell and run bb with the env vars inline)


(defmacro with-env [env & body]
(with-redefs [u/getenv (stub-getenv ~env)]
~@body))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is stubbing the backend side of the pod. Stubbing should happen on the client side of the pod. This will make sure that the right environment is used in case the pod runs in a different environment.

Copy link
Author

Choose a reason for hiding this comment

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

thanks, quite new to babashka so thats good info for the future

@borkdude
Copy link
Contributor

(e.g. start the pod in a different shell and run bb with the env vars inline)

This seems very unlikely as the pod is started by load-pod from within bb.

@MIJOTHY
Copy link
Author

MIJOTHY commented Feb 22, 2022

@MIJOTHY The pod already exposes the (credentials/default-credentials-provider) by default and this falls back on environment variables. Can you explain why you want to use it directly? I have the feeling I'm missing something.

I'd like to make the interface of my script as foolproof as possible. The script will be used by people with various levels of technical ability, and I'd like the script to not have potentially confusing fallback behaviour (they may have set up their aws cli credentials a while ago, and never given it much thought). I can probably guard against this by having an explicit check for the env vars before instantiating the credentials provider, but in the spirit of conveying intent through code, I think it makes sense to make use of the environment-credentials-provider.

It's not a hill I'd die on. I figured it would be good to bring up here.

@borkdude
Copy link
Contributor

I think it would be ok exposing this.

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.

3 participants