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

Add Dockerfile #655

Merged
merged 8 commits into from
Mar 11, 2023
Merged

Add Dockerfile #655

merged 8 commits into from
Mar 11, 2023

Conversation

eberkund
Copy link
Contributor

It would be nice if this tool could also be distributed as a Docker image in addition to the existing packages.

This would make it much easier to build multi-platform images without manually checking the architecture and downloading the right binary.

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (15c1f0d) 97.27% compared to head (2ecfa2a) 97.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
- Coverage   97.27%   97.27%   -0.01%     
==========================================
  Files          15       16       +1     
  Lines        5845     6124     +279     
==========================================
+ Hits         5686     5957     +271     
- Misses        159      167       +8     
Impacted Files Coverage Δ
src/editorconfig.rs 96.41% <0.00%> (ø)
src/lib.rs 97.29% <0.00%> (+0.45%) ⬆️
src/context.rs 98.16% <0.00%> (+0.91%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Sounds useful, thank you!

Maybe this should also have a quick mention in the README about installation and usage?

Dockerfile Outdated Show resolved Hide resolved
@eberkund
Copy link
Contributor Author

Hi, thanks for the quick response. Your project has been very helpful for me.

I opened this PR as a starting point, I'd be happy to update the README as well. Ideally the image can also be published to Dockerhub via GitHub Actions if you're open to it. Do you have any preferred approach for this?

@JohnnyMorganz
Copy link
Owner

No preference. I've never actually used Docker, so not sure. If we need to setup a docker hub account, I can do that once the basic push action is ready. I know github also has a container registry we could push to. Let me know what you need

.github/workflows/docker.yml Outdated Show resolved Hide resolved
.github/workflows/docker.yml Outdated Show resolved Hide resolved

FROM gcr.io/distroless/cc
COPY --from=build /app/target/release/stylua /
CMD ["./stylua"]
Copy link
Owner

Choose a reason for hiding this comment

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

I don't know too much about Docker, but is it worth configuring an ENTRYPOINT as well? It seems that allows it to be used through docker run

Copy link
Contributor Author

@eberkund eberkund Feb 27, 2023

Choose a reason for hiding this comment

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

You can still use docker run with CMD, I think this is the right way. From what I understand ENTRYPOINT is more for when you still want the ENTRYPOINT to run in addition to specifying an additional command which I don't think is the case for a CLI tool.

You can try like this:

$ docker build . -t stylua
$ docker run stylua /stylua "*.lua"
error: no file or directory found matching '*.lua'

@JohnnyMorganz
Copy link
Owner

Also, lets build to the GitHub Container Registry as well just so its available as much as possible https://docs.github.com/en/actions/publishing-packages/publishing-docker-images#publishing-images-to-docker-hub-and-github-packages

@eberkund
Copy link
Contributor Author

eberkund commented Feb 27, 2023

Also, lets build to the GitHub Container Registry as well just so its available as much as possible https://docs.github.com/en/actions/publishing-packages/publishing-docker-images#publishing-images-to-docker-hub-and-github-packages

Hey, I don't think there is any reason to publish to Docker Hub and GHCR and it wouldn't restrict its available having it in one or the other.

Also, I don't really have a strong preference for one over the other.

Copy link
Owner

@JohnnyMorganz JohnnyMorganz left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, looks reasonable to me!
Not sure if there is any way to test this, but I've added the relevant tokens

@eberkund
Copy link
Contributor Author

eberkund commented Mar 8, 2023

Sorry for the delay, looks reasonable to me! Not sure if there is any way to test this, but I've added the relevant tokens

Well I tested the Dockerfile by building it locally. You added tokens for Docker Hub or GHCR? I should update the PR to push to the right repository and I suppose to test I can just modify the triggers or create a test job?

@JohnnyMorganz
Copy link
Owner

Sorry for the delay, looks reasonable to me! Not sure if there is any way to test this, but I've added the relevant tokens

Well I tested the Dockerfile by building it locally. You added tokens for Docker Hub or GHCR? I should update the PR to push to the right repository and I suppose to test I can just modify the triggers or create a test job?

I did it for docker hub. I think it should be fine to trigger an example yeah

@eberkund
Copy link
Contributor Author

eberkund commented Mar 8, 2023

Okay, can you trigger the workflow then? I don't think I have permissions but the workflow should work now.

@JohnnyMorganz
Copy link
Owner

Looks like forks don't have access to repo secrets, so I copied over the files to another PR #658 and ran the action, and looks like its successful: https://github.com/JohnnyMorganz/StyLua/actions/runs/4374456725/jobs/7653906665 https://hub.docker.com/r/johnnymorganz/stylua/tags

@JohnnyMorganz JohnnyMorganz merged commit b79dd08 into JohnnyMorganz:main Mar 11, 2023
@JohnnyMorganz
Copy link
Owner

Thank you!

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.

2 participants