Skip to content

Conversation

mikeland73
Copy link
Contributor

Summary

This pre-mvp integration is mostly just to get something working. It's missing a lot of stuff. Specifically:

  • envsec auto-install
  • error handling (this requires new envsec --json output so we don't have to do clever parsing).
  • Performance. Currently envsec.Env gets called a few times and not cached.

One open question is how generic we want to make the integration. This implementation is pretty coupled.

How was it tested?

envsec set FOO=BAR
devbox run echo \$FOO

func envsecList(projectDir string) (map[string]string, error) {
cmd := exec.Command(
"envsec", "ls", "--show",
"--format", "json",
Copy link
Collaborator

@LucilleH LucilleH Sep 14, 2023

Choose a reason for hiding this comment

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

I'm not sure if we should do this. I worry that the secrets printed can be discovered through some proc file somewhere.

Can we use the envsec go library directly instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under impression we wanted to do CLI integration in order to avoid bloating devbox. This has the benefit of allowing other 3rd parties to integrate as well if we provide a standard interface.

If stderr and stdout are captured in a buffer, is there really any risk the outputs ends up anywhere else?

We can also go the RPC route, but that's more work and feels a bit overkill.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking deeper it might be ok based on this line. So I guess we are good

func envsecList(projectDir string) (map[string]string, error) {
cmd := exec.Command(
"envsec", "ls", "--show",
"--format", "json",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking deeper it might be ok based on this line. So I guess we are good

@mikeland73 mikeland73 merged commit 1943c4b into main Sep 14, 2023
@mikeland73 mikeland73 deleted the landau/envsec branch September 14, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants