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

Added DockerfileLightCaddy & Fixed the Caddyfile #90

Merged
merged 6 commits into from
Nov 18, 2021
Merged

Added DockerfileLightCaddy & Fixed the Caddyfile #90

merged 6 commits into from
Nov 18, 2021

Conversation

kubo6472
Copy link
Contributor

No description provided.

@PhlexPlexico
Copy link
Owner

I am still relatively new to Caddy as well, but my setup was following the setup I had in the wiki for single domains, since this container will only be running one domain. Have you tested the handle_path directive extensively? I know I had some issues with initial testing before reaching the end product [here[(https://github.com/PhlexPlexico/G5V/wiki/Webserver-Setup-With-Caddy) with having to strip prefixes, or you would get 404's if someone went to https://your.get5.instance/api/matches/ (notice the ending /, if that doesn't get removed it 404s). But if you can confirm all the traversing (going to the API, loading app, pressing back button doesn't produce a 404), then this should be good to merge in :)

@PhlexPlexico
Copy link
Owner

Oh! Speaking of, and looking back at the branch you wish to merge into - I now remember the problem that I ran into with Caddy! Since Caddy requires variables for reverse proxy, nostname, and the API endpoint at build time, there is no possible way for this to be created into a package like the NGINX version! That's the one main issue I had when looking into this. However, we can still merge it in, but just know that it won't be at all possible to have this in a docker-compose file due to this issue!

Thanks!

Caddyfile Outdated
uri strip_prefix /API_END_POINT
reverse_proxy {$CADDY_PROXY_URL}
{$CADDY_URL} {
handle_path /{$CADDY_API_ENDPOINT}* {
Copy link
Owner

Choose a reason for hiding this comment

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

Also, the api endpoint path isn't needed in this Caddyfile since it's not acting as the reverse proxy, so these API paths will have to be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To basically respond to all is just that this is the setup I am currently running with no 404s. It's covering the whole stack with a single webserver basically.

Copy link
Owner

Choose a reason for hiding this comment

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

Just so I'm getting this right you mean "one" webserver being the main reverse proxy (in your case I assume Caddy?) that would be setup independent from all projects (excluding the new docker-compose file)? If so, that's good to know! Then I am willing to accept the changes but please make sure to not include the reverse proxy directives in this file as it is not needed inside the container, it should be pushed up one level :) I think you should be able to remove all variables in this file as well, as I've done in the branch you are merging into. If you just rebase onto that and make the merge conflict fixes I should be able to merge both in. Please note that I'm only doing a light file, as a full file should not be needed, and I would like to move away from that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The light file is my choice all the way, everytime. The levels was what I was avoiding. It's this weird hybrid setup of three containers: mariadb, g5api and g5v that provides the dist files and a webserver+reverse proxy all in that third one. I did some experiments assemling it into a compose file, but not successfully, yet. You can think of it as a very monolithic deployment. But very easy for basic setups.

Copy link
Owner

Choose a reason for hiding this comment

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

I mean, if you want an easy setup you should be able to use this one just fine, and it uses Caddy as the reverse proxy https://github.com/PhlexPlexico/G5API/blob/master/docker-compose.yml

But yeah, I think we're on the same page then. It's just cutting down to semantics and I guess if someone wants to roll everything with Caddy, I suppose it should be allowed (even though it doesn't really make any difference at this level). Once resolved, I can merge everything in. Just make sure the Caddyfile stays similar to the changes that were recently made (i.e. no build variables needed) and it should be good to merge :)

Copy link
Owner

@PhlexPlexico PhlexPlexico left a comment

Choose a reason for hiding this comment

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

Rebase will fix everything here, just adjust the Caddyfile to that in the docker-caddy branch and we'll be off to the races!

@@ -0,0 +1,28 @@
FROM caddy:2.4.6-alpine
Copy link
Owner

Choose a reason for hiding this comment

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

So yeah, delete this file please

README.md Outdated
@@ -57,6 +57,32 @@ Now, you can run your application by running ```docker container run --name g5v
G5V requires G5API to be running at `/api`, you can change this behavior run `docker build -t yourname\g5v:latest -f DockerfileFull --build-arg VUE_APP_G5V_API_URL=<your_api_url>` (be sure to include `http(s)://` and not to have a trailing `/`). If you are using Docker Compose refer to [this](https://docs.docker.com/compose/compose-file/compose-file-v3/#args).
Some example setup configs can be found [here](https://github.com/PhlexPlexico/G5V/wiki).

#### DockerfileFullCaddy:
Copy link
Owner

Choose a reason for hiding this comment

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

Just make sure the readme doesn't get changed too much either - a rebase should fix these :)

@PhlexPlexico
Copy link
Owner

Updated the readme to fix the change issue and the Caddyfile to reflect your changes, just delete the dockerfilefullcaddy and we should be good to merge in!

@kubo6472
Copy link
Contributor Author

kubo6472 commented Nov 18, 2021 via email

@PhlexPlexico PhlexPlexico merged commit 67f7c3f into PhlexPlexico:docker-caddy Nov 18, 2021
@kubo6472 kubo6472 deleted the docker-caddy branch November 18, 2021 22:51
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