-
Notifications
You must be signed in to change notification settings - Fork 164
chore(scripts): fix script and supports podman #836
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
Conversation
This comment has been minimized.
This comment has been minimized.
zeitlinger
left a comment
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.
Thanks for the contribution!
b96cb49 to
45d9ea2
Compare
martincostello
left a comment
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.
We should apply the same improvements for our Windows users too 😃
|
Note: I prefer |
900be6b to
123c7ec
Compare
7d3f502 to
1963e58
Compare
Yeah, I wasn't aware of it, so I ran it locally and fixed the issues I found. Let's see what it looks like on GitHub Actions. |
|
Oops... rebased now. |
|
Looks OK to me - will clone locally and run the PowerShell scripts to check it's working. |
martincostello
left a comment
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.
Some changes after testing locally.
- Update
cmdfile to pass boolean to PowerShell correctly. - Fix incorrect
elsein.ps1. - Always use the
$ReleaseTagvariable.
39bd08d to
c8b2a3c
Compare
Most fixes are related to executing the run-lgtm.sh script, but I also changed the build scripts and Docker file. The "container" directory and additional directories required by Grafana, Loki, and Prometheus were not present. The "container" directory is now part of the ".gitignore" file, meaning that if a newcomer downloads this repository and attempts to run the "run-lgtm.sh" script, it won't work. We will create these directories if they don't exist. The second issue comes with the container runtime. I am a Fedora Workstation user, and because of that, it contains some interesting paradigms when running containers. By default, Fedora uses Podman as its container runtime. I didn't switch to Docker for personal reasons. I added some changes related to the Dockerfile to include the image's full address, defaulting to docker.io. I could have used Red Hat's mirror suggested by Podman (registry.access.redhat.com). However, Red Hat has a rate limit on its own registry. Red Hat also publishes its UBI images to Docker Hub. Docker Hub is the most well-known Docker Registry, and some people pay for a Docker license, so they don't get rate-limited. It's a reasonable choice to use Docker Hub. This prevents Podman from prompting the user to select a registry when building the image. Finally, to address a minor issue with Bind Mounts. I added the "z" flag as a mount option, which allows users with SELinux enabled to allow container images to manipulate files on the host machine. SELinux comes enabled by default on Fedora Workstation. Otherwise, the bind mount works but prevents the running container from writing data, resulting in "access denied". The "z" flag won´t be an issue on non-Red Hat systems. I tested this flag on Debian 11 with KVM.
martincostello
left a comment
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.
Just one tiny change to fix lint warning, otherwise LGTM.
Co-authored-by: Martin Costello <[email protected]> Signed-off-by: Willian Braga da Silva <[email protected]>
Most fixes are related to executing the run-lgtm.sh script, but I also changed
the build scripts and Docker file.
The "container" directory and additional directories required by Grafana, Loki,
and Prometheus were not present. The "container" directory is now part of the
".gitignore" file, meaning that if a newcomer downloads this repository and
attempts to run the "run-lgtm.sh" script, it won't work. We will create these
directories if they don't exist.
The second issue comes with the container runtime.
I am a Fedora Workstation user, and because of that, it contains some
interesting paradigms when running containers.
By default, Fedora uses Podman as its container runtime. I didn't switch to
Docker for personal reasons.
I added some changes related to the Dockerfile to include the image's full
address, defaulting to docker.io. I could have used Red Hat's mirror
suggested by Podman (registry.access.redhat.com). However, Red Hat has a
rate limit on its own registry. Red Hat also publishes its UBI images to
Docker Hub. Docker Hub is the most well-known Docker Registry, and some
people pay for a Docker license, so they don't get rate-limited. It's a
reasonable choice to use Docker Hub. This prevents Podman from prompting the
user to select a registry when building the image.
Finally, to address a minor issue with Bind Mounts. I added the "z" flag as
a mount option, which allows users with SELinux enabled to allow container
images to manipulate files on the host machine. SELinux comes enabled by
default on Fedora Workstation. Otherwise, the bind mount works but prevents
the running container from writing data, resulting in "access denied". The
"z" flag won´t be an issue on non-Red Hat systems. I tested this flag on
Debian 11 with KVM.