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

Vikunja update to 0.24.1 #1230

Merged
merged 8 commits into from
Aug 14, 2024
Merged

Conversation

MalteKiefer
Copy link
Contributor

No description provided.

@nmfretz nmfretz closed this Jul 22, 2024
@nmfretz nmfretz reopened this Jul 22, 2024
@nmfretz
Copy link
Contributor

nmfretz commented Jul 22, 2024

Thanks very much for all the app updates and submissions @MalteKiefer! Very much appreciated. We will take a look at these shortly. Please ignore the random close/open from me testing a workflow.

@MalteKiefer
Copy link
Contributor Author

External port is need, there is a mobile app for Vikunja.

Copy link
Contributor

@nmfretz nmfretz left a comment

Choose a reason for hiding this comment

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

Thanks again for this Vikunja update @MalteKiefer. Looks like the Vikunja developers are simplifying the architecture for Docker Compose deployments which is nice.

I've left some comments below with suggested changes. In addition to those changes, we can also delete the entire proxy subdirectory within vikunja/data since the new compose architecture no longer requires nginx.

Once addressed, we can test to make sure the app is working well for fresh installs and app updates.

@@ -32,19 +36,8 @@ services:
VIKUNJA_SERVICE_FRONTENDURL: http://${DEVICE_DOMAIN_NAME}/
Copy link
Contributor

Choose a reason for hiding this comment

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

With the change to a single image for the frontend and api, it looks like VIKUNJA_SERVICE_FRONTENDURL is no longer needed. Although we could add in VIKUNJA_SERVICE_PUBLICURL for good measure:
https://vikunja.io/changelog/whats-new-in-vikunja-0.23.0#using-docker

@@ -22,7 +22,7 @@ description: >-

You can view your tasks in the classic list view - or in a Gantt Chart, or Table view, or Kanban Board. Whatever you need!
releaseNotes:
This is a patch release with many small bug fixes and improvements. Full release notes are available at https://vikunja.io/blog/2024/01/vikunja-frontend-and-api-v0.22.1-was-released/
This is a patch release with many small bug fixes and improvements. Full release notes are available at https://vikunja.io/changelog/vikunja-v0.24.1-was-released
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a huge update for users on umbrelOS since they will be updating from 0.22.1 to 0.24.1.
We could summarize some of the major relevant changes here or just mention that it is a major update from 0.22.1 to 0.24.1. That way users don't think they are just updating to a patch.

Comment on lines 39 to 40
ports:
- 3456:3456
Copy link
Contributor

Choose a reason for hiding this comment

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

External port is need, there is a mobile app for Vikunja.

We shouldn't need to expose a separate port on the host for this to work. Right now the app_proxy is making Vikunja available on port 4523 on the host, with the proxy's auth disabled. So as-is, a user would connect a client to Vikunja with something like http://umbrel.local:4523/... (I'm guessing on the connection format). To make it easier for users, we could change the main port for the app to 3456 in the umbrel-app.yml so that our port would match documentation or forum posts that users may search for online when troubleshooting. So this way users would connect via http://umbrel.local:3456/....

Are you currently running the alpha pre-release mobile app? If so, do you know what url is usually used to connect, and would you be able to help test that it works?

Copy link

🎉   Linting finished with no errors or warnings   🎉

Thank you for your submission! This is an automated linter that checks for common issues in pull requests to the Umbrel App Store.

Please review the linting results below and make any necessary changes to your submission.

Linting Results

Severity File Description
ℹ️ vikunja/docker-compose.yml Potentially using unsafe user in service "db":
The default container user "root" can lead to security vulnerabilities. If you are using the root user, please try to specify a different user (e.g. "1000:1000") in the compose file or try to set the UID/PUID and GID/PGID environment variables to 1000.
ℹ️ vikunja/docker-compose.yml Potentially using unsafe user in service "web":
The default container user "root" can lead to security vulnerabilities. If you are using the root user, please try to specify a different user (e.g. "1000:1000") in the compose file or try to set the UID/PUID and GID/PGID environment variables to 1000.

Legend

Symbol Description
Error: This must be resolved before this PR can be merged.
⚠️ Warning: This is highly encouraged to be resolved, but is not strictly mandatory.
ℹ️ Info: This is just for your information.

@nmfretz
Copy link
Contributor

nmfretz commented Aug 14, 2024

I have gone ahead and made the required changes and tested Vikunja as a fresh install and update from v0.22.1 with data persistence. Thanks again for this PR!

@nmfretz nmfretz merged commit 38232f5 into getumbrel:master Aug 14, 2024
1 check passed
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