-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
add caddy server as system install #1070
Conversation
cmd/system/caddy.go
Outdated
} | ||
|
||
command.Flags().StringP("version", "v", "", "The version or leave blank to determine the latest available version") | ||
command.Flags().String("path", "/usr/local/bin", "Installation path, where a caddy subfolder will be created") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not the usual pattern, but Caddy installs to /usr/bin/
Have a look at the faasd script to see what else may be missing, including adding users etc.
For adding users, we will need to detect the OS from /etc/os-release and reject the command if it's not debian or ubuntu.
In the future, if someone needs this for Alpine or Fedora, they can add a patch to run useradd with the correct syntax for their distro.
There are also some directories created and chmod is run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start, but there's more needed for a system install to match how Caddy is installed with faasd: https://github.com/openfaas/faasd/blob/master/hack/install.sh
The new changes look like what I was expecting. Could you test this and see if Caddy starts up correctly on a new Linux VM you create somewhere? If you need a sample Caddyfile, you can change the domains for the below:
|
cmd/system/caddy.go
Outdated
} | ||
|
||
commands := strings.Join(shellCmds, "; ") | ||
if err = exec.Command(commands).Run(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try go-execute which is used elsewhere? It has a shell option if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cmd/system/caddy.go
Outdated
// TODO: Add empty CaddyFile and if required FAAS_DOMAIN setup too | ||
|
||
shellCmds := []string{ | ||
"$SUDO chown --recursive caddy:caddy /var/lib/caddy", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think $SUDO could be removed, and arkade could be run as sudo arkade
if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cmd/system/caddy.go
Outdated
|
||
caddyUser := "caddy" | ||
if _, err = user.Lookup(caddyUser); errors.Is(err, user.UnknownUserError(caddyUser)) { | ||
addUserCmd := fmt.Sprintf("$SUDO useradd --system --home %s --shell /bin/false %s", caddyHomeDir, caddyUser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try go-execute again which splits out cmd vs args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@nitishkumar71 suggested using the "get" method from the tools code. I think it would be worth trying out as an experiment. |
600ee14
to
94f6f54
Compare
Re-executing system install produced below output
|
cmd/system/caddy.go
Outdated
return err | ||
} | ||
|
||
if _, err = executeShellCmd(context.Background(), "systemctl", "start", "caddy"); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skip the start, and then you can also omit createCaddyConf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return fmt.Errorf("this app only supports Linux") | ||
} | ||
|
||
tools := get.MakeTools() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change mean that "caddy" also appears via arkade get caddy
?
In the case of things like Go or Node, we won't that because it won't do the right thing. We could have an additional flag or bool etc on each Tool to hide them from the arkade get
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. will start working toward migrating system apps to use this new approach and enable apps which we want as system install only
|
||
tools := get.MakeTools() | ||
var tool *get.Tool | ||
for _, t := range tools { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach. Could you create an issue to retrofit this into the existing system install apps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I would try to progressively apply this approach in system install apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you see the suggestions please?
Signed-off-by: Nitishkumar Singh <[email protected]>
94f6f54
to
405fa7b
Compare
Signed-off-by: Nitishkumar Singh <[email protected]>
405fa7b
to
a484094
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
Motivation and Context
design/approved
by a maintainer (required)Closes Add Caddy Server as System install #1069
How Has This Been Tested?
Executed the command
arkade system install caddy
produced below outputIf updating or adding a new CLI to
arkade get
, run:Types of changes
Documentation
./arkade get --format markdown
./arkade install --help
Checklist:
git commit -s