ogham / exa

A modern replacement for ‘ls’.
https://the.exa.website/
MIT License
23.67k stars 661 forks source link

Add Dockerfile to enable running end-to-end tests with docker #1230

Closed AgustinRamiroDiaz closed 1 year ago

AgustinRamiroDiaz commented 1 year ago

Context

Fixes https://github.com/ogham/exa/issues/1225

Reviewer tips

The commit history is a bit dirty (if we go forward we should squash it), so look at the final picture

I also encourage you to run the tests in your local machine to see how it works. Documentation on how to do this in the README.md

Notes

AgustinRamiroDiaz commented 1 year ago

@polarathene thanks a lot for your feedback! I'll be addressing it in my next commits

chaining && \

Where do you think we could use this feature? We don't have the command chaining, we only have some multi-line commands only for readability (install a lot of apt packages)

It's ok to extract some scripts out to their own shell script file if appropriate, sometimes it's just noise to the Dockerfile and doesn't belong there, better linting too. Example.

Yeah I agree, but for this PR I don't want to be too disruptive, I think this is great as a post iterative improvement

You create a few shell files (t, rexa, etc) to run different commands, but could use a single shell script with a switch to support that. Example (not the best).

Sorry, I didn't understand the example. If you mean to modify the current scripts, I do not want to modify the internal behavior because I cannot guarantee that they'll behave correctly in Vagrant (couldn't make it work)

polarathene commented 1 year ago

Where do you think we could use this feature? We don't have the command chaining, we only have some multi-line commands only for readability (install a lot of apt packages)

I see now that you've mostly taken the scripts from the Vagrantfile as-is. I provided some suggestions of where you could use it.

If both files are to be kept, it probably should be it's own script file that is shared between the two. Personally I'd be in favor of removing the Vagrantfile and simplify the `Dockerfile.

Yeah I agree, but for this PR I don't want to be too disruptive, I think this is great as a post iterative improvement I do not want to modify the internal behavior because I cannot guarantee that they'll behave correctly in Vagrant (couldn't make it work)

I haven't looked too much into how those were being used, but since this is just taking lines from the Vagrantfile I can understand why you want to take that approach.

AgustinRamiroDiaz commented 1 year ago

@polarathene @mentalisttraceur thanks a lot for your time and feedback. I think I've addressed it all, please ping me if not

polarathene commented 1 year ago

Thanks for addressing the feedback :)

I'm quite busy so might not have much more feedback for now, especially since further work is being deferred to a future PR.

I haven't tried building and running it yet, but I'm curious if it appears to be working with tests passing? Especially those related to the usage of mknod (might work if that's run at runtime if build time isn't persisting /dev).

AgustinRamiroDiaz commented 1 year ago

I haven't tried building and running it yet, but I'm curious if it appears to be working with tests passing?

@polarathene actually that's my main concern, tests do NOT pass at the moment of writing this, it'd be great if someone with Vagrant could check them to see if it's a problem from the Dockerfile or from the tests. The output is huge so I didn't want to paste it here, but can do it if you need it

polarathene commented 1 year ago

@AgustinRamiroDiaz I don't get a good impression that it'll be easy to resolve that. It may be better to drop pursuing this further.

Are you aware of eza? It's a recent fork of exa where development is moving at a much faster pace and addressing many issues and PRs that have been open / stale for exa.

They've been discussing the test suite as well with its issues, using Docker / Nix and migrating the test suite to something else that isn't as problematic for them.

ariasuni commented 1 year ago

Closing this since exa is unmaintained (see #1243), you should take a look at the active fork eza (an alternative strategy has been used for tests).