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

Change installation path to ~/.kubescape/bin #1147

Merged
merged 5 commits into from Mar 24, 2023
Merged

Change installation path to ~/.kubescape/bin #1147

merged 5 commits into from Mar 24, 2023

Conversation

HollowMan6
Copy link
Contributor

Resolve #1015

Overview

The installation behavior now changes into the follows:

  • If the user tries to start the script with root access, then kubescape will be installed into /usr/local/bin/.
  • If without root access, then kubescape will be installed into ~/.kubescape/bin, and a message will be shown to user asking them to put that directory into PATH manually.

Signed-off-by: Hollow Man <hollowman@opensuse.org>
@craigbox
Copy link
Contributor

That's a good plan.

Do we think people use this script to upgrade Kubescape? Do we need to remove older versions of kubescape from places that were already in the path, first?

If I installed an old version the old way, and then a new version the new way, I might end up with an older version, earlier in the $PATH.

@craigbox craigbox requested a review from dwertent March 14, 2023 20:01
Signed-off-by: Hollow Man <hollowman@opensuse.org>
@HollowMan6
Copy link
Contributor Author

Do we think people use this script to upgrade Kubescape? Do we need to remove older versions of kubescape from places that were already in the path, first?

If I installed an old version the old way, and then a new version the new way, I might end up with an older version, earlier in the $PATH.

Just updated it to fix this issue. If the /usr/local/bin/kubescape is found and user tries to upgrade it without using root, the script will use sudo to remove it. If user tries to use sudo to upgrade but had old installation under home dir, the script will also be able to remove the home dir installation.

One thing to note that I don't know if ${SUDO_USER:-$USER} will work for macOS, as I don't have a mac. @craigbox would you help me test that? Thanks!

@craigbox
Copy link
Contributor

I'm not so much worried as "user installed as root", I'm worried by "user ran the old script, which didn't install in /usr/local/bin but instead installed in whatever the first writable directory in the $PATH there was".

It might be worth iterating through the $PATH and asking the user if they want to delete any kubescapes found?


$ echo ${SUDO_USER:-$USER}
craigbox

Was that what you wanted?
I have a newer bash than most Mac users though. (Worth pointing out that the script uses bash, not zsh, and we always ask people to pipe it to bash.)

@HollowMan6
Copy link
Contributor Author

I'm not so much worried as "user installed as root", I'm worried by "user ran the old script, which didn't install in /usr/local/bin but instead installed in whatever the first writable directory in the $PATH there was".

It might be worth iterating through the $PATH and asking the user if they want to delete any kubescapes found?

Yeah, you are right! That's a kind of technical debt. Will work on this tomorrow.

$ echo ${SUDO_USER:-$USER}
craigbox

Was that what you wanted? I have a newer bash than most Mac users though.

Yes, exactly. I think that shouldn't be a problem as this is not something new.

Signed-off-by: Hollow Man <hollowman@opensuse.org>
@craigbox
Copy link
Contributor

My only concern with scripts like this is we really really need to make sure $KUBESCAPE_EXEC can never be empty.

@HollowMan6
Copy link
Contributor Author

My only concern with scripts like this is we really really need to make sure $KUBESCAPE_EXEC can never be empty.

Ah, yeah, that's really another problem. Will add that check before the deletion command as well.

Signed-off-by: Hollow Man <hollowman@opensuse.org>
@HollowMan6
Copy link
Contributor Author

I think now this is ready to get merged as well.

Copy link
Contributor

@matthyx matthyx left a comment

Choose a reason for hiding this comment

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

just a few shellcheck nits

install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@matthyx
Copy link
Contributor

matthyx commented Mar 24, 2023

@HollowMan6 maybe at the same time you could address the other shellcheck suggestions:

In install.sh line 6:
    case ${option} in
    ^-- SC2220: Invalid flags are not handled. Add a *) case.


In install.sh line 11:
if [ -z ${RELEASE} ]; then
        ^--------^ SC2086: Double quote to prevent globbing and word splitting.

Did you mean: 
if [ -z "${RELEASE}" ]; then


In install.sh line 20:
KUBESCAPE_ZIP=kubescape.zip
^-----------^ SC2034: KUBESCAPE_ZIP appears unused. Verify use (or export if used externally).

Thanks in advance!

@matthyx
Copy link
Contributor

matthyx commented Mar 24, 2023

@HollowMan6 also you probably need to amend locally to please the DCO :-/

Signed-off-by: Hollow Man <hollowman@opensuse.org>
@HollowMan6
Copy link
Contributor Author

Thank you!

@matthyx matthyx merged commit 5923ce5 into kubescape:master Mar 24, 2023
4 checks passed
@matthyx
Copy link
Contributor

matthyx commented Mar 24, 2023

thanks @HollowMan6 !

@HollowMan6 HollowMan6 deleted the install branch March 24, 2023 11:54
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.

kubescape installed in first directory in $PATH under $HOME
3 participants