vikunja
/
frontend
4
29
Fork 37
Code Issues 89 Pull Requests 36 Releases 22 Activity
Labels Milestones

Docker refactoring #3018

Merged
konrad merged 6 commits from vlasov-y/frontend:main into main 2 months ago
Conversation 10 Commits 6 Files Changed 7
vlasov-y commented 2 months ago

Hi! ✋

Your product looks awesome and I have decided to spawn an instance for myself. I have written a Helm chart and tried to deploy your application.
I has failed because of wrong Dockerfile architecture. You run Nginx as non-root user, but do not override cache paths so that it fails to make chown for /var/cache/nginx/*.

First, I have started building crutches using init containers, but then thought that maybe I can participate in open-source and share my experience. I have refactored your frontend. Also I plan to review your backend as well. Then I will add flexible Helm charts for both, if you do not mind :)

Changelog

  1. Splitted pnpm install and build to speed up the build in total
  2. Added optional JSON logging format that can be set using VIKUNJA_LOG_FORMAT env var during the runtime. (check other env variables in the Dockerfile)
  3. Refactored your run.sh and now it is scripts/docker/injector.sh
  4. Carefully copied your custom Nginx configuration in my set, so I should break nothing
  5. Used native Nginx docker-entrypoint.d to run the injector
  6. Switched the whole container to non-root in Dockerfile directly
  7. Built and launched the frontend locally - works
  8. Added Docker Buildx comment in the very beginning of Dockerfile
  9. Added Docker build documentation notes to README.md
  10. Changed command to generate src/version.json (without changing version format, of course) using jq to generate a valid JSON
Hi! ✋ Your product looks *awesome* and I have decided to spawn an instance for myself. I have written a Helm chart and tried to deploy your application. I has failed because of wrong Dockerfile architecture. You run Nginx as non-root user, but do not override cache paths so that it fails to make chown for /var/cache/nginx/\*. First, I have started building crutches using init containers, but then thought that maybe I can participate in open-source and share my experience. I have refactored your frontend. Also I plan to review your **backend** as well. Then I will add flexible **Helm charts** for both, if you do not mind :) ### Changelog 1. Splitted pnpm install and build to speed up the build in total 2. Added optional JSON logging format that can be set using `VIKUNJA_LOG_FORMAT` env var during the runtime. (*check other env variables in the Dockerfile*) 3. Refactored your run.sh and now it is scripts/docker/injector.sh 4. Carefully copied your custom Nginx configuration in my set, so I should break nothing 5. Used native Nginx docker-entrypoint.d to run the injector 6. Switched the whole container to non-root in Dockerfile directly 7. Built and launched the frontend locally - works 8. Added Docker Buildx comment in the very beginning of Dockerfile 9. Added Docker build documentation notes to README.md 10. Changed command to generate src/version.json (without changing version format, of course) using jq to generate a valid JSON
vlasov-y added 1 commit 2 months ago
continuous-integration/drone/pr Build is passing Details
11799a6829 Docker refactoring
vlasov-y added spent time 2 months ago
2 hours
frederick commented 2 months ago
Collaborator

Hi vlasov-y!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3018-main--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi vlasov-y! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3018-main--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
konrad requested changes 2 months ago
konrad left a comment
Owner

Nice work!

The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker

Nice work! The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker
Dockerfile Outdated
# installs usermod and groupmod
shadow
ENV VIKUNJA_HTTP_PORT 8080
ENV VIKUNJA_HTTP2_PORT 8081
konrad commented 2 months ago
Owner

Do we even need to let users change these ports? Isn't it enough to choose a different "outer" port when running and publishing the container?

Do we even need to let users change these ports? Isn't it enough to choose a different "outer" port when running and publishing the container?
vlasov-y commented 2 months ago
Poster

Depends... Maybe someone will want to run the container in host network. If you want, I can just hardcode to 8080 and 8081.

Depends... Maybe someone will want to run the container in host network. If you want, I can just hardcode to 8080 and 8081.
👍 1
Dockerfile Outdated
CMD "/run.sh"
COPY scripts/docker/injector.sh /docker-entrypoint.d/50-injector.sh
COPY scripts/docker/nginx.conf /etc/nginx/nginx.conf
COPY scripts/docker/templates/. /etc/nginx/templates/
konrad commented 2 months ago
Owner

Can we move this from scripts/docker to just docker? Since this now also contains config file I think it's more than just scripts.

Can we move this from `scripts/docker` to just `docker`? Since this now also contains config file I think it's more than just scripts.
vlasov-y commented 2 months ago
Poster

Sure! Usually, I create folder .devops. I will move files to folder named docker, as you asked.

Sure! Usually, I create folder `.devops`. I will move files to folder named *docker*, as you asked.
konrad commented 2 months ago
Owner

I built this container locally but it looks like it does not work:

$ curl localhost:8888
curl: (56) Recv failure: Connection reset by peer

(running as docker run -p 8888:80 vikunja/frontend:dev)

I built this container locally but it looks like it does not work: ``` $ curl localhost:8888 curl: (56) Recv failure: Connection reset by peer ``` (running as `docker run -p 8888:80 vikunja/frontend:dev`)
vlasov-y commented 2 months ago
Poster

I built this container locally but it looks like it does not work:

$ curl localhost:8888
curl: (56) Recv failure: Connection reset by peer

(running as docker run -p 8888:80 vikunja/frontend:dev)

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

docker run --rm -d --name vikunja-test -p 8888:8080 vikunja/frontend:dev
curl -v http://localhost:8888
> I built this container locally but it looks like it does not work: > > ``` > $ curl localhost:8888 > curl: (56) Recv failure: Connection reset by peer > ``` > > (running as `docker run -p 8888:80 vikunja/frontend:dev`) @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. ```shell docker run --rm -d --name vikunja-test -p 8888:8080 vikunja/frontend:dev curl -v http://localhost:8888 ```
vlasov-y added 1 commit 2 months ago
continuous-integration/drone/pr Build is passing Details
827dff09c5 Moved docker files out of scripts folder
vlasov-y requested review from konrad 2 months ago
konrad commented 2 months ago
Owner

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

> @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.
konrad commented 2 months ago
Owner

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

Okay so I just started this with VIKUNJA_HTTP_PORT=80 and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.

> > @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. > > Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke. Okay so I just started this with `VIKUNJA_HTTP_PORT=80` and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.
konrad reviewed 2 months ago
docker/nginx.conf
server_tokens off;
types_hash_max_size 2048;
types_hash_bucket_size 64;
client_max_body_size 500M;
konrad commented 2 months ago
Owner

This seems unnecessary as the frontend does not handle any file uploads. I don't know if it would hurt to leave it in there, though.

This seems unnecessary as the frontend does not handle any file uploads. I don't know if it would hurt to leave it in there, though.
vlasov-y commented 2 months ago
Poster

It is just a part of template I use everywhere. I agree, this should be removed.

It is just a part of template I use everywhere. I agree, this should be removed.
vlasov-y commented 2 months ago
Poster

fixed

fixed
👍 1
konrad marked this conversation as resolved
docker/nginx.conf
image/svg+xml
image/x-icon
audio/wav;
gzip_disable "MSIE [1-6]\.";
konrad commented 2 months ago
Owner

I think we can omit this, we don't support IE at all

I think we can omit this, we don't support IE at all
vlasov-y commented 2 months ago
Poster

Okay

Okay
vlasov-y commented 2 months ago
Poster

fixed

fixed
👍 1
konrad marked this conversation as resolved
konrad reviewed 2 months ago
docker/nginx.conf
'"request_method": "$request_method",'
'"request_time": "$request_time",'
'"server_addr": "$server_addr",'
'"server_name": "$server_name",'
konrad commented 2 months ago
Owner

I think this can be removed since the server name is always _.

I think this can be removed since the server name is always `_`.
vlasov-y commented 2 months ago
Poster

Okay

Okay
vlasov-y commented 2 months ago
Poster

fixed

fixed
👍 1
konrad marked this conversation as resolved
docker/nginx.conf
'"upstream_connect_time": "$upstream_connect_time",'
'"upstream_header_time": "$upstream_header_time",'
'"upstream_response_length": "$upstream_response_length",'
'"upstream_response_time": "$upstream_response_time",'
konrad commented 2 months ago
Owner

Same for the upstream log messages since they are only used when nginx is used as a proxy?

Same for the upstream log messages since they are only used when nginx is used as a proxy?
vlasov-y commented 2 months ago
Poster

Yeah, it is also a part of my template :) Will remove as well

Yeah, it is also a part of my template :) Will remove as well
vlasov-y commented 2 months ago
Poster

fixed

fixed
👍 1
konrad marked this conversation as resolved
vlasov-y commented 2 months ago
Poster

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

Okay so I just started this with VIKUNJA_HTTP_PORT=80 and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.

Okay. It works, but I am confused. Nginx works as non-root o_O

image

> > > @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. > > > > Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke. > > Okay so I just started this with `VIKUNJA_HTTP_PORT=80` and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup. Okay. It works, but I am confused. Nginx works as non-root o_O ![image](/attachments/cc3c350f-7b98-479f-9024-ea5e87808b0b)
image.png
66 KiB
vlasov-y added 1 commit 2 months ago
continuous-integration/drone/pr Build is passing Details
d3b0876054 Removed unnecessary Nginx configuration
vlasov-y added 1 commit 2 months ago
continuous-integration/drone/pr Build is failing Details
fd1844c924 Changed Docker port to 80 by default
vlasov-y commented 2 months ago
Poster

@konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one.

Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke.

Okay so I just started this with VIKUNJA_HTTP_PORT=80 and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup.

done

> > > @konrad You have published the wrong port. Container listens 8080 inside, not 80. Use this one. > > > > Can we change this so that it listens on port 80 by default? I know that's not as straightforward as your solution but it does this by default and changing the port will break a lot of existing settings. It's easy to fix but I fear most people won't read the changelog and then complain in issues and the forum how their setup broke. > > Okay so I just started this with `VIKUNJA_HTTP_PORT=80` and it just worked. Please change the default value for the env variable, that should be enough to not break people's setup. done
vlasov-y added 1 commit 2 months ago
continuous-integration/drone/pr Build is passing Details
3cd0bedd1b Fixed http2 port
vlasov-y added 1 commit 2 months ago
continuous-integration/drone/pr Build is passing Details
5cb678263f Switched to nginx user
konrad approved these changes 2 months ago
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit e4499f44b7 into main 2 months ago
konrad referenced this issue from a commit 2 months ago
Docker refactoring (#3018)
konrad commented 2 months ago
Owner

Okay. It works, but I am confused. Nginx works as non-root o_O

I'm not sure how they do it, but this stack exchange answer says it is possible. The nginx container probably did this.

> Okay. It works, but I am confused. Nginx works as non-root o_O I'm not sure how they do it, but [this stack exchange answer says it is possible](https://superuser.com/a/892391). The nginx container probably did this.
vlasov-y commented 2 months ago
Poster

Nice work!

The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker

Could you please point me to the documentation repository? I am not quite sure where is it.

> Nice work! > > The docs need adjustment: https://vikunja.io/docs/install-frontend/#docker Could you please point me to the documentation repository? I am not quite sure where is it.
konrad commented 2 months ago
Owner

The docs are here: https://kolaente.dev/vikunja/api/src/branch/main/docs

I've already adjusted the docs about the puid and gid of the Frontend container.

The docs are here: https://kolaente.dev/vikunja/api/src/branch/main/docs I've already adjusted the docs about the puid and gid of the Frontend container.
👍 1
viehlieb referenced this issue from a commit 2 months ago
Docker refactoring (#3018)

Reviewers

konrad approved these changes 2 months ago
continuous-integration/drone/pr Build is passing
Details
The pull request has been merged as e4499f44b7.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
konrad
Labels
Apply labels
Clear labels
area/internal-code changes requested dependencies duplicate
This issue or pull request already exists help wanted
Need some help hosting invalid
Something is wrong kind/bug
Something is not working kind/feature
New feature question
More information is needed wontfix
This won't be fixed
No Label area/internal-code changes requested dependencies duplicate help wanted hosting invalid kind/bug kind/feature question wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
4 Participants
Notifications
Total Time Spent: 2 hours
vlasov-y
2 hours
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3018
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'vlasov-y/frontend:main'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes