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

OS support #1732

Merged
merged 24 commits into from
Feb 23, 2024
Merged

OS support #1732

merged 24 commits into from
Feb 23, 2024

Conversation

nemchik
Copy link
Member

@nemchik nemchik commented Feb 23, 2024

Pull request

Purpose
Follow up from #1731 to make DS compatible with CoreOS (or any OS where a supported package manager is not detected.

Approach
This results in DS exiting with a fatal when any required dependency is missing (and a supported package manager is not found to install the dependency) and informing the user that they need to install said dependency manually.

Requirements
Check all boxes as they are completed

@nemchik nemchik requested a review from a team as a code owner February 23, 2024 01:31
@github-actions github-actions bot added the core Automatic label label Feb 23, 2024
@dylanmtaylor
Copy link
Contributor

The only thing I'm not a fan of is no longer providing the user the link for how to install compose.

Signed-off-by: Eric Nemchik <eric@nemchik.com>
@nemchik
Copy link
Member Author

nemchik commented Feb 23, 2024

The only thing I'm not a fan of is no longer providing the user the link for how to install compose.

Yeah I thought a bit about removing that and decided it would be nice to keep it the same as the others in terms of wording and verbosity. Technically there are many ways one could install compose as well as any of the other dependencies.

@dylanmtaylor
Copy link
Contributor

That page gives a bunch of different options. Including the exact command you need on coreos.

@dylanmtaylor
Copy link
Contributor

If you use two targets for home, if either isn't in the path, it looks like it will warn even if nothing is wrong and it would work fine.

@nemchik nemchik changed the title Os support OS support Feb 23, 2024
@dylanmtaylor
Copy link
Contributor

Can we add the docker compose doc I linked to the readme at least? It's probably exactly what the user needs to do.

@nemchik
Copy link
Member Author

nemchik commented Feb 23, 2024

If you use two targets for home, if either isn't in the path, it looks like it will warn even if nothing is wrong and it would work fine.

This is true. I also found many OS don't include $HOME/bin or $HOME/.local/bin in $PATH by default unless it exists before/when their bash profile is loaded, so in theory some users might just need to log out and back in or reboot and there would not be an issue. I am open to suggestions on the wording.

Can we add the docker compose doc I linked to the readme at least? It's probably exactly what the user needs to do.

I am definitely fine with the link being added to the readme with a description somewhere, and I'm not dead set that it can't be included in the failing command output, but maybe it would be cleaner as a warn right before the fatal, what do you think?

maybe

            if ! docker compose version > /dev/null 2>&1; then
                warn "Please see https://docs.docker.com/compose/install/linux/ to install 'docker compose'"
                fatal "Error: 'docker compose' is not available. Please install 'docker compose' and try again."
            fi

Signed-off-by: Eric Nemchik <eric@nemchik.com>
@nemchik nemchik merged commit eb0a2e0 into master Feb 23, 2024
21 checks passed
@nemchik nemchik deleted the os-support branch February 23, 2024 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Automatic label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants