search

rojopolis / spellcheck-github-actions

Spell check action
MIT License
138 stars 38 forks source link

Docker improvements #12

Closed jonasbn closed 4 years ago

jonasbn commented 4 years ago

Experimented with Alpine without success, some of the XML requirements did not work.

jonasbn commented 4 years ago

I have added a branch with my Alpine work, ref:

edumco commented 4 years ago

I've try the docker image in a few repositories and here here my test notes:

1) Readme could to be updated to point to the docker hub image

2) Still needed to create a spellcheck.yml file with little information about its contents. A separate tutorial explaining in details should be created.

3) The example on reamdme has the worlist.txt listed so when we run without creating the file an error appears.

Could be a informative message like:

You have declared a $WORDLIST_FILE_NAME file in your spellcheck but it was not found!

Remove the file declaration from spellcheck or create the file with the same name.

4) The error message doesn't count correctly the numbers of files with errors

Misspelled words:
<htmlcontent> resources/frameworks/cucumber.md: html>body>ul>li>p
--------------------------------------------------------------------------------
UI
--------------------------------------------------------------------------------

Misspelled words:
<htmlcontent> resources/frameworks/jasmine.md: html>body>ul>li
--------------------------------------------------------------------------------
Aprenda
Javascript
Unitários
--------------------------------------------------------------------------------

!!!Spelling check failed!!!
(1) Files in repository contain spelling errors

5) The html tags in the log might be confusing. It's not easy to transpile markdown into html using the brain :P

Later I post comments about the Dockerfile

edumco commented 4 years ago

My notes on the debian image

1) Debian-slim is great!

2) --no-install-recommends

In debian based images its a good practice to add --no-install-recommends to avoid download extra-packages. If some of these packages are really necessary they should be explicitly as a depend

RUN apt-get update \
    && apt-get install -y --no-install-recommends aspell aspell-en \
    && rm -rf /var/lib/apt/lists/*

3) Pin pip versions using pip freeze

Its a good practice to pin the exact version used for each package and its dependences and put on a requirements.txt.

This forces the docker image build to aways produce the same result.

Today on python:3.8-slim the command pip install pyspelling && pip freeze creates the following list:

backrefs==4.3
beautifulsoup4==4.9.1
bracex==2.0
html5lib==1.0.1
lxml==4.5.1
Markdown==3.2.2
pyspelling==2.6
PyYAML==5.3.1
six==1.15.0
soupsieve==2.0.1
wcmatch==6.0.1
webencodings==0.5.1
edumco commented 4 years ago

About Docker alpine

1) You can have multiple Dockerfiles

Dockerfile
Dockerfile.python37
Dockerfile.python37-alpine11
Dockerfile.python38-alpine11

2) In alpine we don't need to update

Just use the --no-cache and the apk will be found download and installed without touching the cache.

Ex: apk add --no-cache package

3) Temporary package installation

Sometimes we just gonna use a package during build or installation (gcc) so there are 2 approachs:

Ex: RUN apt install unzip && unzip myfile.zip && apt uninstall unzip

4) Building wheel for lxml takes a huge amount of time.

This doeesn't feel right. i believe it could be some bad config in lxml setup.py.

5) Alpine is so good at making small images. Dont give up!

github-action-spellcheck:alpine 106MB github-action-spellcheck:latest 211MB

jonasbn commented 4 years ago

Hi @edumco

Thanks for the elaborate feedback. I have compiled your comments to this list:

jonasbn commented 4 years ago

Improvements and issues, which will be addressed separately have been migrated to separate issues. Documentation has been updated and things have been slimmed down.

Many areas could do with further improvements: