-
Notifications
You must be signed in to change notification settings - Fork 3
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
add bash demo script #75
add bash demo script #75
Conversation
Thanks for addressing my comments! |
--network=host \ | ||
-v "basil-db-vol:/BASIL-API/db/sqlite3" \ | ||
-v "basil-ssh-keys-vol:/BASIL-API/api/ssh_keys" \ | ||
-v "basil-tmt-logs-vol:/var/tmp/tmt" \ |
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'm sorry I just figured that out...
Volumes need to be used in the api container not in the app.
API is the only project that is able to interact with the DB.
--network=host \ | ||
-v "basil-db-vol:/BASIL-API/db/sqlite3" \ | ||
-v "basil-ssh-keys-vol:/BASIL-API/api/ssh_keys" \ | ||
-v "basil-tmt-logs-vol:/var/tmp/tmt" \ |
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'm sorry I just figured that out...
Volumes need to be used in the api container not in the app.
API is the only project that is able to interact with the DB.
|
||
podman run \ | ||
-d \ | ||
--privileged \ |
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.
privileged option is not needed for app container
privileged is required for api to be able to spin up containers inside the api container
|
||
podman run \ | ||
-d \ | ||
--privileged \ |
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.
privileged option is not needed for app container
privileged is required for api to be able to spin up containers inside the api container
for element in $list; | ||
do | ||
exists="$(podman volume exists "${element}")" # check if volume already exists | ||
if [ "${exists}" ]; then |
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.
seems that $exists is empty string all the times
I'm trying to run
podman volume exists "${element}"
and then checking the return value
if [ $? == 0]; then
#exists
I added a script file to deploy the application on a Raspberry Pi as an example of centralized server that anyone can reach in a private network as part of #78 |
closing in favor of #78 where you took over this script without giving me credits, not nice! I tested #78 and it works. I do not see why you made publicity for rpi, it's working out of the box on my Linux Mint 22.1 as well, so I would expect it to work on any hw with debian based linux without any problem. Rather than making this rpi specific I would suggest to make it debian specific and and a automated test where you run this script on each commit in CI. |
@nopeppermint I don't think there is any reason to close this PR. I'm sorry, let me clarify. I give all the credits for the testing and for the idea of the script to you, really. Thanks for testing #78 as well, and the idea of putting it into the CI is really great. Moreover I found that some changes are needed to run it on Debian, you can see the details in the PR. I'll be at FOSDEM next week, presenting BASIL at SBOM track. |
I am fine with closing this MR as #78 adds everything I was suggesting here, I think #78 is even better as it extends this MR. I do agree with your code comments. For next time, I would value a simply "based on MR xyz" in a MR were you take over code or suggesting changes directly in the MR, or merging the MR as is and then extent it. Let's close this and move forward together ;-)
nice, I am not there sorry. |
Make sense, I renamed #78 as per your suggestions. |
not sure, my intent with this MR was to lower the barrier to get BASIL working on my own pc to start evaluating it, not sure if you really need two different docker files for api, but if then I would suggest name them accordingly, Containerfile-api-fedora and Containerfile-api-debian. the run_demo_rpi.sh you could rename to run_demo.sh and add an option which api container to start. I really don't see why rpy is needed there, it's basically a debian docker container. I would more vote for a comment in the documentation pointing out that there is this run_demo.sh script which allows you to easily setup a working BASIL instance on your local pc, there you can chose if you would like a debian or fedora based image, while the debian based one also tested on a rpy but should work on any debian based linux. As you have this script now, you might want to change the CI to also use this script and test all it's options automatically. |
No description provided.