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

Sablier binary shouldn't be located on its current location #348

Closed
gtherond opened this issue Jul 5, 2024 · 6 comments · Fixed by #352
Closed

Sablier binary shouldn't be located on its current location #348

gtherond opened this issue Jul 5, 2024 · 6 comments · Fixed by #352
Labels

Comments

@gtherond
Copy link

gtherond commented Jul 5, 2024

Describe the bug
The current container build use /etc/sablier/sablier as the binary location once it have been built

Context

  • Sablier version: 1.7.0 and upper
  • Provider: irrelevant in here
  • Reverse proxy: irrelevant in here
  • Sablier running inside a container? Yes

Expected behavior
As the Dockerfile is using an Alpine image for the final layer, it should try to respect Alpine and Linux FHS as much as possible.

If the build have to follow the requirements of either Linux or Alpine, the resulting binary should be located on one of the following available path:

  • /bin/sablier
  • /sbin/sablier
  • /usr/bin/sablier
  • /usr/local/bin/sablier
  • /usr/local/sbin/sablier
  • /opt/sablier
  • /opt/sablier/sablier

Additional context
Per see, there is no real big issue, BUT, this helps sablier to be more solid.
I can do the patch and PR if you're willing to change the binary location.

TBN: The sablier yaml configuration file have to stay on /etc/sablier.yaml or /etc/sablier/sablier.yaml to match Linux FHS.

TBN2: Sablier being a static golang binary, the Dockerfile final layer could even be derivated from scratch and the binary placed on one of the expected upper suggested path, that would reduce even more the perimeter to support with this container image.

@gtherond gtherond added the bug Something isn't working label Jul 5, 2024
@acouvreur
Copy link
Owner

You can open a PR, I agree with all your points.

Maybe for the scratch image, this can be tedious to debug because you won't have a shell image inside.

What do you think ?

acouvreur added a commit that referenced this issue Jul 8, 2024
…blier

As the Dockerfile is using an Alpine image for the final layer, it should try to respect Alpine and Linux FHS as much as possible.

Note that the config file will remain in /etc/sablier/ folder

Fixes #348
@acouvreur
Copy link
Owner

I simply changed the binary location for this PR.

For making the image scratch, I don't know yet. Depends if we still want to have a shell inside I guess.

I'm also interested in having this container rootless if you have any expertise. I think it's quite complicated given the fact we need docker group to access the socket.

I could something like Linuxservers.io images with PGID and PUID.

acouvreur added a commit that referenced this issue Jul 8, 2024
…blier

As the Dockerfile is using an Alpine image for the final layer, it should try to respect Alpine and Linux FHS as much as possible.

Note that the config file will remain in /etc/sablier/ folder

Fixes #348
@acouvreur
Copy link
Owner

🎉 This issue has been resolved in version 1.8.0-beta.10 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gtherond
Copy link
Author

gtherond commented Jul 9, 2024

I simply changed the binary location for this PR.

For making the image scratch, I don't know yet. Depends if we still want to have a shell inside I guess.

I'm also interested in having this container rootless if you have any expertise. I think it's quite complicated given the fact we need docker group to access the socket.

I could something like Linuxservers.io images with PGID and PUID.

Perfect!
You're awesome, thanks a lot.

Yep, regarging the scratch base, it obviously isn't a light hearted decision and yes I didn't had those docker socket requirements in mind as I make it run on a kubernetes context. I could definitely have a look and find out how would it behave with a Wolfi or Google distroless base. I'll test it and let you know.

@gtherond
Copy link
Author

gtherond commented Jul 9, 2024

You can open a PR, I agree with all your points.

Maybe for the scratch image, this can be tedious to debug because you won't have a shell image inside.

What do you think ?

Sorry I just saw your answer and find out you had done it two days ago, I'll enable notifications ^^

Thanks again!

@acouvreur
Copy link
Owner

🎉 This issue has been resolved in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants