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

New script: Outline LXC #472

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

burgerga
Copy link

@burgerga burgerga commented Nov 23, 2024

Note

We are meticulous when it comes to merging code into the main branch, so please understand that we may reject pull requests that do not meet the project's standards. It's never personal. Also, game-related scripts have a lower chance of being merged.

Description

Add a community script to install Outline as discussed in #342 (comment).

Type of change

Please check the relevant option(s):

  • Bug fix (non-breaking change that resolves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (a fix or feature that would cause existing functionality to change unexpectedly)
  • New script (a fully functional and thoroughly tested script or set of scripts.)

Prerequisites

The following efforts must be made for the PR to be considered. Please check when completed:

  • Self-review performed (I have reviewed my code, ensuring it follows established patterns and conventions)
  • Testing performed (I have tested my changes, ensuring everything works as expected)
  • Documentation updated (I have updated any relevant documentation)

Additional Information (optional)

  • You can test with
    bash -c "$(wget -qLO - https://github.com/burgerga/ProxmoxVE/raw/add_outline/ct/outline.sh)"
    This works because I changed the urls to the community repo in the 2nd commit, this needs to be reverted before merge.
  • Update also works (tested with v0.79 -> v0.81.1, and nothing happens with v0.81.1 -> v0.81.1)
  • Perhaps controversial: this script requires a URL to be set for the app. Since the installation takes kind of long, I moved all user interaction as much to the front as possible. That way you provide some user input at the start, and then you can go do something else, grab a coffee, etc. This will prevent the process being stuck halfway, waiting for some avoidable human input.
  • Please check if the information in the json is enough: this app will not function without 1) reverse proxy that does https, and 2) identity provider that needs to be added to /opt/outline/.env.
  • Script works both on Debian 12 and Ubuntu 24 (didn't test others), should I add both to the json?

Related Pull Requests / Discussions

#342 (comment)

If there are other pull requests or discussions related to this change, please link them here:

  • Related PR #

@burgerga burgerga requested a review from a team as a code owner November 23, 2024 21:42
@github-actions github-actions bot added new script A change that adds a new script website A change to the website high risk A change that can affect many scripts labels Nov 23, 2024
Copy link
Member

@MickLesk MickLesk left a comment

Choose a reason for hiding this comment

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

Hey,

Thanks for submitting a new script.
I've left a few comments, check them out.
And next time, make sure you don't push the build and Install.fuc ;)

ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
install/outline-install.sh Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
misc/build.func Outdated Show resolved Hide resolved
misc/install.func Outdated Show resolved Hide resolved
json/outline.json Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Show resolved Hide resolved
@burgerga
Copy link
Author

Hi @MickLesk, thanks for your time and effort reviewing this! I have responded to your comments, and already pushed a few fixes. Let me know if further changes are needed. Before merging I can rebase (I'm not sure I can do that already without messing up the review process).

@havardthom havardthom changed the title Add outline New script: Outline LXC Nov 24, 2024
@MickLesk
Copy link
Member

Hi @MickLesk, thanks for your time and effort reviewing this! I have responded to your comments, and already pushed a few fixes. Let me know if further changes are needed. Before merging I can rebase (I'm not sure I can do that already without messing up the review process).

Checkout @havardthom's suggestions, then i take a final look into it :-)

Copy link
Member

@MickLesk MickLesk left a comment

Choose a reason for hiding this comment

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

I have added a few comments based on the last pulls. The script can be simplified in some places and in a few places it breaks with our standards. Nothing dramatic. But it should be adjusted and then tested.

ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
ct/outline.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
install/outline-install.sh Outdated Show resolved Hide resolved
@burgerga
Copy link
Author

Hi @MickLesk, @havardthom,
I have made the requested changes. Just to be sure:

  • APPLICATION should not be used in the install.sh?
  • What about the backup? I think this is a nice-to-have in case upstream changes cause an update to fail.
  • Is the JSON alright?
    • the reverse proxy requirement is similar to Vaultwarden, where I copied the message from
  • The outline author did update the instructions, but it's not completely reflecting the Dockerfile, which I think is a better reference.

I have tried several things to try and get the application accessible via http://localhost:3000 but somehow I just can't get it to work.
An alternative solution would be to make this a post-install thing:

  1. Use emptyURL= in the .env file -> starting the app will fail since it requires this (or use URL=http://localhost:3000, but better letting it fail probably)
  2. In post-install, people should:
    • Set URL in /opt/outline/.env
    • Add authentication options to /opt/outline/.env to enable login
    • Run yarn db:create to create the database
    • Run systemctl restart outline

@burgerga
Copy link
Author

burgerga commented Nov 27, 2024

Added #556, let me know your thoughts. You can try an install with

bash -c "$(wget -qLO - https://github.com/burgerga/ProxmoxVE/raw/add_outline_new/ct/outline.sh)"

Still need to update the json, will the link stay the same, when you make it into announcement?

@MickLesk
Copy link
Member

I've been looking at the script for 2 hours now.

  1. is the DB still missing? or is it really only created via “yarn sequelize db:create”? Normally we do the following:
$STD sudo -u postgres psql -c “CREATE ROLE $DB_USER WITH LOGIN PASSWORD ‘$DB_PASS;$STD sudo -u postgres psql -c “CREATE DATABASE $DB_NAME WITH OWNER $DB_USER ENCODING ‘UTF8’ TEMPLATE template0;$STD sudo -u postgres psql -c “ALTER ROLE $DB_USER SET client_encoding TO ‘utf8’;$STD sudo -u postgres psql -c “ALTER ROLE $DB_USER SET default_transaction_isolation TO ‘read committed’;$STD sudo -u postgres psql -c “ALTER ROLE $DB_USER SET timezone TO ‘UTC’”
  1. what is Adminer for? I find it unnecessary. Otherwise you would have to add it for every script with MySQL / Postgres.

  2. the .env should actually be adapted or extended?

But in the end I can say:

  • I can't test it. It must be released via a global / public address, which I personally do not do. I couldn't even access it via Cloudflare. Not even in develop mode.

A lot of effort went into it, but I have no idea, if I can't get it to work properly, how can a user?

@community-scripts/contributor feel free to give input

@burgerga
Copy link
Author

burgerga commented Nov 29, 2024

Hi @MickLesk, thanks for your (continued!) effort looking into this! Regarding your questions:

  1. Yes, the DB is still missing, this is indeed handled via yarn db:create. It is dependent on some variables in the .env, and dependent on if/how you want to do SSL (see https://github.com/outline/outline/blob/746e65e65819d001a5be3f9f338953a316b2b5d1/server/storage/database.ts#L16)
  2. That can be removed, it's just in there because I took the linkwarden script as basis for the outline script, but I think it indeed might make more sense as MISC script that can add this on top of existing LXCs/VMs.
  3. Both:
    • Adapted: the URL needs to be provided
    • Extended: some form of authentication needs to be added (for Authentik this was quite simple by copying the OIDC variables from .env.sample and filling those in)

Regarding your last point: Yes, this is frustrating. I tried several things and couldn't get it to accept connection via IP:3000 :(
On the other hand, I use Nginx Proxy Manager to do all of my internal name resolution, and
image
(where I partially blacked out my URL), works flawlessly (so it's not exposed publicly!). Two other people have confirmed it works: #342 (reply in thread) and #342 (reply in thread) (the last one using an URL with Cloudflare tunnel)

@MickLesk
Copy link
Member

I've already considered whether I should submit an issue to the developers. It's nonsense to only enable this via a URL with a forced api (discord, Google, ...). It would make sense for a local installation to be possible with a local user.

Even the change to development in the .env (that it runs locally) cannot be called up in my own network. But I get correct feedback via curl. (From the same machine)

@MickLesk
Copy link
Member

Another idea would be (just spun) to install a nginx/Caddy or Apache2 on it and see if it forwards it correctly.

@burgerga
Copy link
Author

burgerga commented Nov 29, 2024

Yes, I agree, and there is a looong issue about this, that is now locked :P outline/outline#1881 (near the end, they discuss a workaround by using dex to create single user, or put a "fake" identity provider somewhere in the database. )

@burgerga
Copy link
Author

Even the change to development in the .env (that it runs locally) cannot be called up in my own network. But I get correct feedback via curl. (From the same machine)

Yes, I also saw from netstat that it's indeed not a listening issue, so it must be somewhere in the code that prevents this. But haven't find the right portion of the code yet that deals with this...

@MickLesk MickLesk removed the high risk A change that can affect many scripts label Nov 29, 2024
@havardthom
Copy link
Contributor

I also spent a couple of hours yesterday trying to set this up and I could not get it to work. Set it up with a proxy (not exposed) and tried adding my keycloak as the authenticator, but still stuck with an authentication_required error.

Unfortunately since this script requires so much post install setup which is advanced, I think it will cause a lot of issues and maintenance. Personally I don't think we should add this script. If Outline decides to add regular email/password login, maybe we could add it.
Also want to add that I appreciate the work you have put in, I don't want to discredit that. Happy to hear thoughts from other @community-scripts/contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new script A change that adds a new script website A change to the website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants