-
Notifications
You must be signed in to change notification settings - Fork 786
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
Build Prototype Monkey Island AppImage #1069
Conversation
Adds the basic scripts for generating an AppImage. Code changes are required to allow Monkey Island to operate on a read-only filesystem.
1. Rewrote in pytest 2. Removed reduntant tests 3. Added tests for add_user() and get_users()
The `get_from_json()` and `get_from_dict()` static methods were really just used for testing. The `EnvironmentConfig` class needs to store its file path so it can wite to the file if needed. In practical usage, `EnvironmentConfig` objects are initialized from files, so a simpler interface is for its constructor to take a file path.
1. Adjusted some spacing and indentation 2. Reformatted with Black
…a todo, to move it to our fixture list
sudo apt-get install -y nodejs | ||
} | ||
|
||
install_build_prereqs() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add sudo apt update
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VM should be upgraded before building. I'll add details in a readme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's docker or something, how do you know that the repositories will be updated? I'd rather we have sudo apt update
in scripts than specify in the readme that "you need to run sudo apt update".
requirements_island="$ISLAND_PATH/requirements.txt" | ||
# TODO: This is an ugly hack. PyInstaller is a build-time dependency and should | ||
# not be installed as a runtime requirement. | ||
sed '4d' $requirements_island | sponge $requirements_island |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line_with_pyinstaller=$(cat $requirements_island | grep -i -n pyinstaller)
|
||
requirements_island="$ISLAND_PATH/requirements.txt" | ||
# TODO: This is an ugly hack. PyInstaller is a build-time dependency and should | ||
# not be installed as a runtime requirement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this applies to virtualenv as well
# | ||
# Note that appimage-builder replaces the interpreter on the monkey agent binaries | ||
# when building the AppDir. This is unwanted as the monkey agents may execute in | ||
# environments where the AppImage isn't loaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that monkeys can only be executed on the island or what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, this is the explanation to why you download the binaries after building the AppDir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. There are better solutions to this problem, but since we're not using AppImage Builder in the long run, they're not worth investing in.
Version=1.10 | ||
Type=Application | ||
Name=Monkey Island | ||
Comment=FILL ME IN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fill him in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppImage Builder doesn't actually use this file, but different packaging methods do. I'll fill this in in round 2 or remove this file.
@@ -0,0 +1,41 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain when this file is used and when AppRun is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AppRun is part of the AppImage spec. It's the entry point. AppImage Builder generates its own AppRun from the exec
and exec_args
fields in monkey_island_builder.yml
. ATM, AppRun isn't used, so I'll just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phenomenal quality of bash scripts. Some duplication with linux deployment scripts and maybe that can be resolved in the future.
Left some questions, some arise due to the lack of knowledge about how appimage works, others are general suggestions.
} | ||
|
||
install_nodejs() { | ||
NODE_SRC=https://deb.nodesource.com/setup_12.x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you move links like these to the top of the file, it will be easier to fix once they get broken, since you won't have to scroll the whole file. Not worth holding up the PR though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having it localized, as that function is the only place it's used, but I see your point. I'll leave it for now, as this may get reworked anyway in version 2.
## Building an AppImage | ||
|
||
1. Create a clean VM or LXC (not docker!) based on Ubuntu 18.04. | ||
1. Update the OS with `sudo apt update && sudo apt upgrade` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why this shouldn't be a part of build script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably yes, but in its current form, no. I'll add it to the build script. We can revisit it later.
1. Execute `./build_appimage.sh`. This will pull all necessary dependencies | ||
and build the AppImage. | ||
|
||
NOTE: This script is intended to be run from a clean VM. You can also manually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woulnd't it be easier in terms of integration testing, to NOT require a new machine each time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LXCs are very quick to spin up. So are VMs in the right environment. Regardless, the README contains instructions on how to clean up after the script runs so that you don't need to use a fresh environment each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments and questions, but can be solved later/ignored
What does this PR do?
Adds a deployment script that builds Monkey Island as a prototype AppImage.
The changes to the python code have already been reviewed in the following PRs:
These changes were rebased on the develop branch.
The only changes that have not been reviewed are the ones in the
deployment_scripts
directory.PR Checklist
Testing Checklist