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 a SVGO Docker image for Simpleicons formatting #1532

Merged
merged 5 commits into from Jun 9, 2020

Conversation

oleg-nenashev
Copy link
Contributor

Just a PoC for the discussion. This is a Docker image I used to render the image according to the feedback in #1518 . If somebody is interested, I could work a bit on this image to provide a tool for other image contributors who are not very familiar with SVG formats.

What could be added:

CC @ericcornelissen

@oleg-nenashev oleg-nenashev changed the title Add a SVGO Docker image Add a SVGO Docker image for Simpleicons formatting Jul 15, 2019
@ericcornelissen
Copy link
Contributor

I'm not necessarily against supporting this, however I have two doubts:

  • Having a Dockerfile in the root of this project signals to me that it's going to build the website for you and run a local Jekyll server for you to test the website without having to install Ruby on your machine. It might be cool to have a Dockerfile for that though 🤔
  • I'm not sure how many of our contributors will make use of this anyway. I'm not trying to downplay the skills of our contributors but many of them are new to open-source and coding in general so I'm not sure if many of them would use it. Though that is not a reason to not include it!

To circumvent my first point I'm thinking of instead adding the Dockerfile as a link in the CONTRIBUTING.md under 3. Optimize the icon. Still, the Dockerfile needs to be part of the repository most likely... Perhaps we can place the Dockerfile under ./scripts instead to make it a bit clear that it is not for the website?

As for the .dockerignore, it seems a bit arbitrary to me... I get why you don't want to include all the icons, but why ignore the README.md and not all other files in the project? The only thing you really need is .svgo.yml and the image itself, installing svgo should be done using npm install svgo --global instead if we're worried about performance when deciding not to include all the icons, Thoughts?


Ps. I'm assuming PoC means Proof of Concept?

@ericcornelissen ericcornelissen added the meta Issues or pull requests regarding the project or repository itself label Jul 21, 2019
@casperdcl

This comment has been minimized.

@ericcornelissen

This comment has been minimized.

@ericcornelissen ericcornelissen mentioned this pull request Dec 3, 2019
4 tasks
@oleg-nenashev
Copy link
Contributor Author

Sorry, missed the updates

To circumvent my first point I'm thinking of instead adding the Dockerfile as a link in the CONTRIBUTING.md under 3. Optimize the icon. Still, the Dockerfile needs to be part of the repository most likely... Perhaps we can place the Dockerfile under ./scripts instead to make it a bit clear that it is not for the website?

Yes, it would make sense. Also, it could be just put in a separate repository so that there is separate versioning for it. Can help with maintaining it, because I would be interested to convert this image into a GitHub Actions step at some point

Ps. I'm assuming PoC means Proof of Concept?

Yes. Mostly for the review by maintainers to see whether there is value in that

@casperdcl
Copy link
Contributor

I would be interested to convert this image into a GitHub Actions step at some point

A GitHub action serving what purpose? For CI tests or for end-users somehow?

Update the Dockerfile to create a docker image that is generally 
applicable to run NPM commands, including but not limited to:

- npm run test
- npm run svgo
- npm run lint

Also updated the .dockerignore file to exclude:

- The node_modules folder
- Common Jekyll folders/files
- Files generated by the build script

The reason for choosing the alpine docker image (rather than a node 
docker image) is that the CLI out of the box is better.
@ericcornelissen
Copy link
Contributor

ericcornelissen commented Jun 4, 2020

Given the original was a POC and contributed almost a year ago, I took the liberty of updating this PR to our needs. Let me know if you, @oleg-nenashev, or anyone else has any feedback on my solution (as I'm somewhat of a newbie when it comes to Docker)

The Dockerfile in the repository can now be used to create a Docker container for interacting with the Node aspects of the project. You can start a container from this image which you can attach to to run e.g. npm test. Or, as I describe in the updated Contributing Guidelines, to optimize SVGs from outside the container and get the optimized result back on your machine. Scratch that, I adopted the original idea from 3299338

In addition, I added a description for how to run a Jekyll server for this project using a Docker container in the updated Contributing Guidelines. This is unrelated to the Dockerfile that is added in this Pull Request. Instead it uses the jekyll-docker image and mounts this projects root to the containers working directory.

Update the Dockerfile based on the original work in 
3299338 by re-adding an ENTRYPOINT to 
the Dockerfile. This ENTRYPOINT makes it extremely easy to spin up a 
quick Docker container to optimize a single SVG (much simpler than my 
copy-in -> optimize -> copy-out approach).

The description for how to use the Docker image to run other NPM scripts 
has been updated accordingly. The provided command overrides the above 
ENTRYPOINT by simple starting a shell so the user can interact with the 
project.
@PeterShaggyNoble
Copy link
Member

Not familiar with Docker but the updates to the guidelines look good to me 👍

@ericcornelissen
Copy link
Contributor

Given the enormous delay this PR has already seen and given that I merely added a few things to the original PR, I will be merging this now. If someone wants to improve upon this feel free to open a new PR for that.

If anyone still wants to share some feedback on the final Dockerfile, feel free to do so here 🙂

@ericcornelissen ericcornelissen merged commit 0756e1d into simple-icons:develop Jun 9, 2020
@oleg-nenashev
Copy link
Contributor Author

I slightly changed the focus a while ago, but I can help with maintenance of the images if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues or pull requests regarding the project or repository itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants