Skip to content

Fix secrets adapters failing fetch on Windows #1350

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

akumajoe
Copy link

Wanted to implement secret fetching as described in this video: https://youtu.be/sPUk9-1WVXI?si=uqoqUZx-IHrp_fxD&t=526

However, on my Windows environment, this call failed due to the nullification of stderr via {command} 2> /dev/null whenever the CLI was checked. Was able to fix by using the system method instead. This should work on any OS, and retain the same behavior as before, though I haven't checked on Mac or Linux.

(true and false were also issues in testing, so i replaced with exit 0 and exit 1)

@akumajoe akumajoe marked this pull request as draft January 18, 2025 19:41
@akumajoe akumajoe marked this pull request as ready for review January 29, 2025 22:27
@djmb
Copy link
Collaborator

djmb commented Apr 21, 2025

Hi @akumajoe - can you deploy with Windows with these changes? I think there are lots of things in Kamal that assume a Unix-like environment.

You'd probably need to run deployments under the WSL in which case these changes wouldn't be needed I think.

@akumajoe
Copy link
Author

Hey, @djmb! Sorry, haven't been keeping tabs on this, but yes last I messed with this these changes were necessary to use kamal for deployment with Bitwarden as my secrets manager. Issue originated purely from using the backtick shorthand to call system commands as opposed to calling the system method, which is OS agnostic as far as I can tell.

I'm also aware that RoR prefers Mac/Linux in general; I just have a Windows machine and prefer not using WSL if I don't need to. This was the only thing truly blocking my deployment for my test apps, so figured I'd just put a PR out there.

I'm sure I can fix whatever conflicts have cropped up since my last merge down, though yeah, if y'all aren't comfortable merging this in for the sake of accommodating Windows, I can remove this PR.

@djmb
Copy link
Collaborator

djmb commented May 1, 2025

Hey @akumajoe - no I've no objection to changing this, I was just surprised that it is the only thing that's stopping it working from Windows, but I guess things assuming Linux are all on the application host side!

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.

2 participants