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

SECZ-1582: Add Linux support #18

Merged
merged 1 commit into from
Jul 15, 2024
Merged

SECZ-1582: Add Linux support #18

merged 1 commit into from
Jul 15, 2024

Conversation

rbamos
Copy link
Contributor

@rbamos rbamos commented Jul 9, 2024

Testing:

  • Tested locally at the repo root
  • Tested locally one step up from repo root (to check that utils.sh fetch works)
  • Tested install on Linux box
  • Tested install via Ansible; see https://github.com/panorama-ed/ansible-services/pull/5
  • Started configuration, confirmed that Leapp CLI correctly communications with Leapp
  • Fully configured a Leapp installation with config.sh

Core objective: make this script work on both Linux & Mac

Additional tweaks:

  • Utils script is correctly fetched when not present
  • Config broken out into a separate step to facilitate Ansible installation
  • Non-interactive mode created to facilitate Ansible installation
  • Small cleanup

@rbamos rbamos marked this pull request as ready for review July 12, 2024 16:20

# If using an M1 machine, load shell environment to run brew commands
if [[ $(uname -m) == 'arm64' ]]; then
echo ‘# Set PATH, MANPATH, etc., for Homebrew.’ >> ~/.zprofile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unicode Quote -> ASCII quote

@rbamos rbamos force-pushed the SECZ-1582-Add-Linux-Support branch 2 times, most recently from 79b95a5 to d3a2746 Compare July 15, 2024 13:14
setup.sh Outdated
eval "$(/opt/homebrew/bin/brew shellenv)"
fi

if [[ "$kernel_name" == "Linux" ]]; then
(echo; echo 'eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"') >> ~/.bashrc
Copy link
Contributor

@eswidler eswidler Jul 15, 2024

Choose a reason for hiding this comment

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

Do we need to modify ~/.bashrc? I know it looks like we did something similar with the mac version of the script. But if we're only loading shell env vars to be able to run "brew" commands for the duration of this script, is it really necessary to modify our default shell behavior?

Could we just drop this line and use eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)" below?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also Linuxbrew is not installed currently on the eng image. How can we ensure that linuxbrew is installed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)" sets up the current shell session to run brew, so you need to add it to .bashrc for it to apply to every shell session

@eswidler
Copy link
Contributor

An overall comment:
One thing I'm learning is that when logged in as administrator, the home directory does not exist. So anything using ~/... would reference a non-existing dir.

How are you imagining this is run? As an AnsibleAdmin user that still has it's own /home dir & sudo-level access?

setup.sh Outdated
(echo; echo 'eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"') >> ~/.bashrc
eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"
sudo apt install -y build-essential
brew install gcc
Copy link
Contributor

@eswidler eswidler Jul 15, 2024

Choose a reason for hiding this comment

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

Could we do sudo apt install gcc instead? If we can install gcc without brew, we may not need linuxbrew at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. Actually this code is unreachable and we could just remove it outright too -- the Brew installation if check above first checks if we're on Mac OS, so if we're on Linux Brew doesn't get installed and this code doesn't run

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need Linuxbrew for other purposes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't run anything setting up the eng image that required Linuxbrew.

@eswidler
Copy link
Contributor

eswidler commented Jul 15, 2024

Any apt package will be installed on the root volume and not needed if we're creating another workspace based on an image using that volume. Given that, do we need to have all of the sudo apt install PACKAGE lines we have? Could we instead rely on these packages already being available on the root volume?

EDIT: I remember that we have the uninstall process here for testing the setup from scratch. That would require reinstalling the apt packages when reinstalling. I'm ok with leaving these installation calls for now even though they're likely redundant on ansible. Seems low priority to resolve for now.

setup.sh Outdated Show resolved Hide resolved
Comment on lines +33 to +38
# Remove leapp
sudo dpkg -r leapp
sudo dpkg -P leapp
# Remove session-manager-plugin
sudo dpkg -r session-manager-plugin
sudo dpkg -P session-manager-plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you combine the remove and purge flags to run this once for each package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, dpkg complains if you do

@rbamos
Copy link
Contributor Author

rbamos commented Jul 15, 2024

An overall comment:
One thing I'm learning is that when logged in as administrator, the home directory does not exist. So anything using ~/... would reference a non-existing dir.

How are you imagining this is run? As an AnsibleAdmin user that still has it's own /home dir & sudo-level access?

This runs as the ansible user. I did add

  if [[ "$kernel_name" == "Linux" ]] && [[ ! -e "/home/$(whoami)" ]]; then
    sudo mkdir -p "/home/$(whoami)"
    if id -gn | grep 'users' > /dev/null; then
        group='users'
    else
        group=$(id -gn | cut -d ' ' -f 1)
    fi
    sudo chown -R "$(whoami):$group" "/home/$(whoami)"
  fi

But this doesn't actually get run. I could add move it to a spot where it would get run

@eswidler eswidler requested review from eswidler and removed request for eswidler July 15, 2024 17:00
@rbamos rbamos force-pushed the SECZ-1582-Add-Linux-Support branch from d7ecb71 to a054497 Compare July 15, 2024 17:13
@rbamos rbamos merged commit 8e77964 into main Jul 15, 2024
2 checks passed
@rbamos rbamos deleted the SECZ-1582-Add-Linux-Support branch July 15, 2024 17:13
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