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

support embed command in dotenv file #55

Closed
wants to merge 7 commits into from

Conversation

DarthPestilane
Copy link

@DarthPestilane DarthPestilane commented May 17, 2018

Support embed commands via $() (not supported on Windows)

START_TIME=$(date)

Note that using $() might not work depending on your shell.

@hairyhenderson
Copy link
Contributor

IMO it would be best if this were an opt-in feature, since this can open up all kinds of security risks!

@DarthPestilane
Copy link
Author

Agreed.

@DarthPestilane
Copy link
Author

So how about I do it this way:

godotenv.EnableEmbed()
godotenv.Load()

If we don't EnableEmbed() or DisableEmbed() before Load(), godotenv's not gonna parse the embed.

if strings.HasPrefix(value, "$(") && strings.HasSuffix(value, ")") {
cmdStr := value[2 : len(value)-1]
cmd := exec.Command("/bin/sh", "-c", cmdStr)
out, _ := cmd.Output()
Copy link

Choose a reason for hiding this comment

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

Why silence the error here? Wouldn't that make it harder to diagnose failures?

if shouldParseEmbed {
if strings.HasPrefix(value, "$(") && strings.HasSuffix(value, ")") {
cmdStr := value[2 : len(value)-1]
cmd := exec.Command("/bin/sh", "-c", cmdStr)
Copy link

Choose a reason for hiding this comment

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

Shouldn't you do a check for the OS here and if on Windows create a PowerShell session?

@the-wondersmith
Copy link

@durandj @DarthPestilane seeing as it looks like this went stale, I've addressed the comments / concerns and opened a new PR #230

@joho
Copy link
Owner

joho commented May 9, 2024

Closing per #182 but thank you

@joho joho closed this May 9, 2024
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 this pull request may close these issues.

6 participants