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

Feat: Optional change installation directory #2710

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

LEstradioto
Copy link
Contributor

@LEstradioto LEstradioto commented Jun 28, 2024

/claim #2708

@Silvea12 raised a concern about installing Coolify on an immutable filesystem, such as openSUSE MicroOS

Changes made

  • Updated all references from /data/coolify to retrieve the value from .env, allowing users to specify a custom path.

  • Example installation command:
    curl -fsSL https://cdn.coollabs.io/coolify/install.sh | env COOLIFY_ROOT_PATH=/custom/path/to/coolify bash

  • Users can now change the base directory if needed.


Observations for openSUSE MicroOS (self-installing host)

edit: READ HERE TOO

Even with this patch, setting up on this OS can be challenging. The following steps worked for me:

  1. Change the ID attribute from opensuse-microos to opensuse-tumbleweed:
transactional-update shell
# Edit /usr/lib/os-release
  1. Install Docker before running install.sh:
transactional-update pkg in docker
sudo usermod -aG docker $(whoami)
sudo service docker start
  1. Modify SSH configuration:
echo "PermitRootLogin yes" | sudo tee /etc/ssh/sshd_config.d/coolify.conf
  1. Install necessary packages:
transactional-update pkg in curl wget git jq
  1. Comment or delete these lines from install.sh:
# zypper refresh >/dev/null
# zypper install -y curl wget git jq >/dev/null
  1. Run install.sh

Further improve

If approved, I can further improve install.sh, and reduce this steps, and to support more OSes by using ID_LIKE for derivatives, as I saw in this issue

Breaking Changes

  • BASE_CONFIG_PATH in .env must be renamed to COOLIFY_ROOT_PATH.

@peaklabs-dev
Copy link
Member

Does this also work on NixOS: https://nixos.org/ ?

@TapGhoul
Copy link

TapGhoul commented Jun 28, 2024

The install steps above actually can break MicroOS - you don't want to be changing the release as it can cause parts of the system that differentiate between Tumbleweed and MicroOS to misbehave - so we definitely want to specify more aliases above. You also don't need to set up the docker group, as the intent is generally to just have a root user if you're running it on a server, so there's not a lot of point. Just needs to be tweaked a little.

That being said, at least with a quick glance over it, this seems to cover most of what is needed minus documentation and some additional changes I'd suggest adding to install.sh (which I have listed at the end)

My own testing steps - these are not ideal either, but they work for now:

# Set up system - install docker and nano (or your editor of choice) + coolify dependencies, remove podman if installed (not required but having 2 runtimes makes things fight sometimes - ideally we get native podman support at some point) as well as other dependencies
transactional-update run sh -c 'zypper ref && zypper dup -y && zypper in -y docker nano curl wget git jq && zypper rm -y podman'
# Reboot into new OS snapshot
reboot

# Enable docker
systemctl enable --now docker
# Set up SSH stuff
echo -e '# Coolify requires root login over SSH to manage this server\nPermitRootLogin yes' > /etc/ssh/sshd_config.d/70-coolify.conf

# Clone the repo
git clone https://github.com/LEstradioto/coolify.git -b Feat--Customize-Install-Directory
cd coolify
nano scripts/install.sh
# Comment out the zypper-related lines as mentioned
# Add opensuse-microos as a supported OS in the $OS_TYPE check for the support list - this should be fixed in this PR, see below for suggestions

# Start it up!
mkdir /var/coolify
COOLIFY_ROOT_PATH=/var/coolify ./scripts/install.sh

It still references /data for some stuff here, but this is an issue of it still pulling from the CDN - as a result, I could not test end to end, as it'd require modifying the install script substantially, and I don't have time to spend on testing to that degree right now.

A few suggested improvements for @LEstradioto:

  • Add a flag or variable that allows skipping of things like dependency installation and platform checks, maybe ALLOW_UNSUPPORTED_PLATFORMS=1 or something, which instead prints a warning and says "Hey if these are missing this may fail, be sure these are available" - a trapdoor for those who are doing funky stuff is always appreciated ❤️
  • Optionally: add explicit support for opensuse-microos in all areas - if it's microos, add these to the script:

    Note, this may be a bad idea unless MicroOS is a platform that support should be dedicated to going forward - it's fairly niche, so I'd just add the trapdoor and call it a day, rather than adding explicit support.

    transactional-update pkg in -y curl wget git jq
    transactional-update apply
  • Add the SSH lines to /etc/ssh/sshd_config.d/70-coolify.conf on all platforms - all major distributions support reading from this path in their default installs, so there's no reason to modify the primary sshd_config file when we can have overrides written here.
  • Move or change the writable path check - this will always fail in its current state, as a directory not existing (which it won't at first in most cases) will fail this check. Could also just check the exit code of mkdir -p "$COOLIFY_ROOT_PATH" to the if check, as it'll return OK even if it exists already - it'll only fail if it's not writable.

If the user doesn't have docker installed, this should probably be managed by the user, not by the install script.
Additionally, the install of curl + wget + git + jq should probably be in a toolbox container or something (MicroOS's fully mutable temporary environment for doing things like this) but it seems to only support docker as a runtime when run with -S (sandboxed), which prevents it being used for this purpose.

With the above changes, it should make the installation a little smoother for people on MicroOS (and potentially other systems too re: the config file change for sshd)

@TapGhoul
Copy link

@ayntk-ai While the patchset could help with NixOS packaging, due to the complexity of the change supporting NixOS may be out of scope of the bounty I have put on this issue. The only real key change to this is finishing the prior work on allowing a custom host installation directory. This is by no means a "Support all immutable OSes" bounty, just a key part in making support possible at all. The average user doing curl file.sh | bash is not the average user using things like MicroOS or NixOS, so I wouldn't expect native support.

@LEstradioto
Copy link
Contributor Author

Thanks for the informations.

The error you got after successful install.sh is because code changes must reflect on Prod. That should succeed with no worries if merged.
For now, I think you could clone my repo and run like this:

cp scripts/install.sh install.sh
cp scripts/upgrade.sh upgrade.sh
sed -i 's/^CDN=.*/CDN="."/g' install.sh
sed -i 's/^CDN=.*/CDN="."/g' upgrade.sh
COOLIFY_ROOT_PATH=/var/coolify ALLOW_UNSUPPORTED_PLATFORMS=1 SKIP_DEPENDENCY_INSTALLATION=1 ./install.sh 4.0.0-beta.307

Ive already did a check on the writable of root path, so it fails if no write permission

We could add opensuse-microos. @Silvea12 could help me adding aliases (perhaps check ID_LIKE too)
But, I agree that for any explicit support we should wait @andrasbacsai position

Changes made:

  • Refactor encapsulation to install.sh
  • Added new flags, ALLOW_UNSUPPORTED_PLATFORMS, SKIP_DEPENDENCY_INSTALLATION
  • About ssh lines, actually install.sh do not change the PermitRootLogin, so I just updated the warning message suggesting the override on /etc/ssh/sshd_config.d/70-coolify.conf

@LEstradioto
Copy link
Contributor Author

LEstradioto commented Jun 28, 2024

I did a refactor on install.sh too and added Logo and Colors 😄 @andrasbacsai

@TapGhoul
Copy link

TapGhoul commented Jun 29, 2024

Due to the sheer number of changes in the second commit, I'm hesitant to even test this properly to see if it passes the needs of the original issue, as I may be testing code paths that may be canned. @andrasbacsai can you comment on this change? Not really sure what to do here, the second commit to this PR makes a lot of behavioral changes. I'm going to put this on ice on my end and wait for a comment back from you.

Worth a note too, your mentioned instructions don't work, because changing the CDN URL to pull from the local dir doesn't work when fed into curl.

@LEstradioto
Copy link
Contributor Author

Sorry guys.

I will turn this into Draft. I did more stuff then the requested, my bad. The correct is to separate these in another PRs. So I will do this first

@LEstradioto LEstradioto force-pushed the Feat--Customize-Install-Directory branch from 5fcfab8 to 5e1d603 Compare July 1, 2024 20:39
@LEstradioto
Copy link
Contributor Author

Okay, that's it. I undo the last commit.

The command now is like this:
COOLIFY_ROOT_PATH=/another/path SKIP_OS=1 sudo -E scripts/install.sh

Only one flag now, just SKIP_OS (this skips os check and dependencies installs). As there are very few cases where using them separately or individually is necessary.

Copy link

gitguardian bot commented Jul 1, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

@TapGhoul
Copy link

TapGhoul commented Jul 4, 2024

Aside from $SKIP_OS being an unintuitive flag name to me (but that's personal preference), LGTM? I'm happy to pay out for this on merge by maintainers (once they run their own review)

@TapGhoul
Copy link

TapGhoul commented Aug 9, 2024

Is there any plan to go forward with this @andrasbacsai? I cannot pay out any bounty until this is sorted.

@andrasbacsai
Copy link
Member

A lot of things change since then, so this needs to be reviewed @LEstradioto (sorry about that).

@LEstradioto LEstradioto closed this Oct 5, 2024
@LEstradioto LEstradioto force-pushed the Feat--Customize-Install-Directory branch from ea2b6fb to 128d732 Compare October 5, 2024 01:10
@LEstradioto LEstradioto reopened this Oct 5, 2024
@LEstradioto
Copy link
Contributor Author

Okay, rebased to next and kept variable named BASE_CONFIG_PATH, so no breaking changes, if any

User can opt-in BASE_CONFIG_PATH and MANUAL_DEPENDENCIES

Installing like this:
curl -fsSL https://cdn.coollabs.io/coolify/install.sh | env BASE_CONFIG_PATH=/custom/path/to/coolify MANUAL_DEPENDENCIES=true bash

Would install Coolify with a customized installation path and skip automatic dependencies installation.
Probably must update docs to Advanced Installation or something

But, hey @andrasbacsai :D!
I do have a doubt in line 513 and 516

Every path on codebase was /data/coolify/ and I have no idea why its /_data/coolify/ and /_data/databases/ in those lines

Even though its only in dev, how to match using the right config?

@peaklabs-dev peaklabs-dev added the 🛠️ Feature Issues requesting a new feature. label Oct 14, 2024
@github-actions github-actions bot removed the 🛠️ Feature Issues requesting a new feature. label Nov 15, 2024
@peaklabs-dev peaklabs-dev marked this pull request as draft November 15, 2024 11:19
@peaklabs-dev peaklabs-dev added the 🛠️ Feature Issues requesting a new feature. label Nov 19, 2024
@peaklabs-dev
Copy link
Member

@LEstradioto Are you still working on this, if you do not have time I totally understand. Please let me know so I can close this, if not you can always open a new PR in the future.

@peaklabs-dev peaklabs-dev added the 💤 Waiting for feedback Issues awaiting a response from the author. label Dec 16, 2024
@LEstradioto LEstradioto force-pushed the Feat--Customize-Install-Directory branch 2 times, most recently from 138e34b to c24bba7 Compare December 18, 2024 18:07
@TapGhoul
Copy link

I'm still down to pay out the bounty when merged, by the way.

@LEstradioto LEstradioto force-pushed the Feat--Customize-Install-Directory branch 2 times, most recently from 5d36095 to 9f12b00 Compare December 18, 2024 19:17
@LEstradioto LEstradioto force-pushed the Feat--Customize-Install-Directory branch from 9f12b00 to 6a32769 Compare December 18, 2024 19:22
@LEstradioto
Copy link
Contributor Author

@LEstradioto Are you still working on this, if you do not have time I totally understand. Please let me know so I can close this, if not you can always open a new PR in the future.

Hey @peaklabs-dev ! Actually it is done. Ive synced just now to next.

Note: In dev mode, altering the installation directory may cause issues (there are lines with hardcoded paths such as lines 513 and 516). Since only in dev, I wouldnt consider an issue?

@peaklabs-dev peaklabs-dev marked this pull request as ready for review December 18, 2024 20:39
@peaklabs-dev peaklabs-dev removed the 💤 Waiting for feedback Issues awaiting a response from the author. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛠️ Feature Issues requesting a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants