Make option for excluding directories

[Rotendahl]

Hi.
I was looking for an action to remove CRLF's from file and found your repo.

Right now the action does not work for my project.
I have some staticfiles built by the django framework that causes the action to fail.
I figured a good solution would be a way to exclude directories from the action.

I've added a parameter "exclude" which takes a space separated list of directors to
accept CRLFs in.
I also fixed the todo such that the exit code is the number of files with CRLF.

This is the first time I'm playing with GitHub actions, so I'm not sure how to test it.
Let me know if you have any improvements I normally work with python and have not done a lot of bash scripting.

[erclu]

I'm kinda swamped right now so i really can't follow this thing but feel free to experiment! When I'll have time i'll merge your work and i will tag it as v2.

I also fixed the todo such that the exit code is the number of files with CRLF.

oh thats great, thanks!

This is the first time I'm playing with GitHub actions, so I'm not sure how to test it.

Me neither, I was using the test branch to trigger individual runs and checking the results manually (at the time i was not that experienced with github actions either)

Let me know if you have any improvements I normally work with python and have not done a lot of bash scripting.

yeah i'm not that expert with bash either. though I'm pretty sure you can implement this feature with a python script, modifying the dockerfile to include a python distribution and changing the entry point to your "entrypoint.py". the performance impact should not be too significant.

I've added a parameter "exclude" which takes a space separated list of directors to
accept CRLFs in.

I think a more standard way to do this would be to accept space (or comma?) separated globs, which will also cover the "i want to exclude a single file" case. if you do switch the action to python i think this would be a great way to implement it:
https://docs.python.org/3/library/pathlib.html#pathlib.Path.glob

[Rotendahl]

I'll maybe take at look at it in the weekend.

I think I'll stick with bash, it's good practice for me to do code in another language than python.
I'll also experiment with testing.

I've maybe found a better solution than an exclude parameter.
Instead we could add a boolean parameter respect_gitignore.

The problem in my project is that some of the staticfiles generated by django has CLRF endings.
They are not in git because the static directory is in .gitignore. If the parameter is true the script would check if any files with CLRF is in gitignore and if so still return 0.

It struck me that I actually could use the actions as is, if i moved the check before the build stage.

I like the respect_gitignore idea better than the exclude parameter. What do you think?

[erclu]

It struck me that I actually could use the actions as is, if i moved the check before the build stage.

yeah I believe this would be better for your specific use case. You could also move the action to a separate job in the same workflow

I've maybe found a better solution than an exclude parameter.
Instead we could add a boolean parameter respect_gitignore.

honestly i'd stick with the glob-based exclude which allows for more flexibility in all possible use cases. I think this might be a good solution:

https://www.gnu.org/software/bash/manual/bash.html#Filename-Expansion

The GLOBIGNORE shell variable may be used to restrict the set of file names matching a pattern. If GLOBIGNORE is set, each matching file name that also matches one of the patterns in GLOBIGNORE is removed from the list of matches. If the nocaseglob option is set, the matching against the patterns in GLOBIGNORE is performed without regard to case.

[jmckenna]

@Rotendahl I also need to exclude directories. This PR would be extremely helpful.

[erclu]

@Rotendahl I also need to exclude directories. This PR would be extremely helpful.

I had to take a break but I should be able to check this out and merge it in the next days. I'll update the v1 tag so feel free to use that if you wanna get it asap

[jmckenna]

great thanks @erclu

[jmckenna]

Testing v1 now. Hmm, even without adding any 'exclude' the results seem to be odd messages such as:

find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
find: 'file': No such file or directory
\033[1;32mNo files with CRLF endings found.\033[0m

See log at: https://github.com/jmckenna/MapServer/runs/1152302619?check_suite_focus=true

I think there is a problem @Rotendahl . This was working before merging this into v1. Thoughts?

[jmckenna]

I think we should revert these changes (as any deployment projects leveraging v1 now will all get these 'find' messages and passing tests, when it should fail on CRLF).

Thoughts?

Maybe keep v1 for pre-today, and use v1.1.0 to test the changes in this PR? (or "v1.1.0-dev")

Let me know how to proceed.

Oh should I use v1.0.0 for the live sites, as we test this PR?

[jmckenna]

Ok falling back to v1.0.0 gives the expected results (meaning: before this PR was implemented). (see example expected results with v1.0.0: https://github.com/jmckenna/MapServer/runs/1152365919?check_suite_focus=true

[erclu]

I think we should revert these changes (as any deployment projects leveraging v1 now will all get these 'find' messages and passing tests, when it should fail on CRLF).

Thoughts?

Maybe keep v1 for pre-today, and use v1.1.0 to test the changes in this PR? (or "v1.1.0-dev")

Let me know how to proceed.

Oh should I use v1.0.0 for the live sites, as we test this PR?

yeah I'm sorry about this i kinda hoped that everything was good because i'm not that familiar with bash. I think i'll just tag this as v2 and work (slowly) on fixing it. I'll put the v1 tag back to v1.0.0 so as to not break the action for people already using it

[jmckenna]

thanks @erclu, I'll follow along the progress and test the exclude changes when its ready