Closed JeremiahSecrist closed 4 months ago
Thanks for the kind words!
I'm thinking if it would make sense to just make the whole deployment command configurable, so you can set it to whatever you like. This might also address #4 in one go. What do you think, would separate useSudo
and sshArgs
be easier to use than the complete command?
Thanks for the kind words!
I'm thinking if it would make sense to just make the whole deployment command configurable, so you can set it to whatever you like. This might also address #4 in one go. What do you think, would separate
useSudo
andsshArgs
be easier to use than the complete command?
Thanks for replying! I'm thinking a bit of both approach. Common use cases should be accounted for without adjusting the command directly. This helps maintain readability and is what I see in every service deployed to nixpkgs. So having a separate option for useSudo or a similarly named argument is a must.
That being said I often see nixpkgs services have a section open for extra arguments.
So the workflow would look something like: Args for common options -> Ags for extra custom options -> overwrite the entire command.
What do you think, would separate
useSudo
andsshArgs
be easier to use than the complete command?
With my current logic on display they should have separate options, As I can see many using sudo for deployments and many using agents / other ssh args.
I was thinking further about the structure
The config file would look as such:
{
lollypops.deployment = {
sudo = {
package = pkgs.sudo; # default and optional
command = "sudo";
enable = true; # false by default
opts = [ "-E" ]; # empty by default
};
ssh = {
package = pkgs.ssh; # this is the default and optional to the user
command = "ssh";
user = "admin"; # the default here would be root
opts = [ "-p" "2222" ]; # empty by default.
};
};
}
This would allow for a lot of customization as well as have sane defaults.
As always, let me know what you think!
Sounds fine to me, the defaults make sense and the new options would not break existing configurations as far as I can tell. Thanks for the input!
I can implement this in the next days, or, if you want, you can submit a PR with this to get it sooner :)
I actually have a pr in the works I just need to test some things. It would break someone's existing config files but it does make them more readable. Also, the package options are not needed in this situation given the current way it works. Feel free to look at my fork in the meantime!
I took the liberty to look at your fork @arouzing, good job. I second that this feature would be great and I'm looking forward to have your future PR merged into master. In the mean time, would it be possible for you to also modify the chown
command of line 40
to use sudo
as it also seems to require root privileges?
{{.REMOTE_COMMAND}} {{.REMOTE_OPTS}} {{.REMOTE_USER}}@{{.REMOTE_HOST}} "chown ${x.owner}:${x.group-name} ${pkgs.lib.escapeShellArg x.path}"
I took the liberty to look at your fork @arouzing, good job. I second that this feature would be great and I'm looking forward to have your future PR merged into master. In the mean time, would it be possible for you to also modify the
chown
command of line40
to usesudo
as it also seems to require root privileges?{{.REMOTE_COMMAND}} {{.REMOTE_OPTS}} {{.REMOTE_USER}}@{{.REMOTE_HOST}} "chown ${x.owner}:${x.group-name} ${pkgs.lib.escapeShellArg x.path}"
Should be easy to add!
Nice, thanks for the work already! Looking forward to the PR :+1:
I took the liberty of adding support in #14, based on your initial changes @arouzing but augmented a bit and extended.
Hope you don't mind, as it had been a while since there was any activity on this 🙇
Thank you very much for submitting this as a PR!
Fixed by #14
@cmacrae There is a quite serious bug, i believe it comes from this line
When deploying with the newest version, my home folder /home/pinpox
gets the permissions set to 0077
instead of 0700
. Did you mean to write 700
there or do we need another check?
I'm trying to hotfix it, but haven't found the solution yet, if you know the answer let me know so we can fix this before anything breaks, otherwise I will have to temporarily revert this before someone runs into problems until it is fixed.
EDIT: I pushed a fix in f1f865f41ac7b433da0960b44322acb4d196d1a5 changing the permissions set by install
from 077
to 700
, which is more reasonable I think. If you find the time, have a look and let me know if I missed something, 2 pairs of eyes on this is probably safer.
I've been busy with things and just had not been able to get back around too it. I'll pass on the torch so to speak to @cmacrae.
Fixed by commit mentioned in previous comment. If any further problems come up, feel free to open a new issue or reopen
Love the project so far and its simplicity! I cant wait to see how it grows!
useSudo Useful with systems that deploy using a non root users.
sshArgs: Having a way to pass through various args is useful especially when dealing with locked down systems and jump boxes.
Overall adding these options makes securing the deployments easier.
Let me know if you have any questions regarding how these features might work.